From f2ca4eb3a6619cdf8050388acde0e781f56f6dcb Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:02:19 -0700 Subject: [PATCH 01/16] fix(setup): honest, identifier-safe error reporting across the setup CLIs (SONA-200) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Operator-facing failures in setup, connect-domains, and apply-download-ratelimit now state their real cause instead of guessing: - Token-scope blame appears only on 401/403, through one shared cfFailureTail. A 2xx whose body says success:false repeats the API's own code and message, a network failure says the API did not respond, and any other status carries the API's reason attributed as 'the API said …' so our advice and Cloudflare's words stay separable. - Cloudflare error bodies never print raw. cfErrorSummary allowlists code + message, strips control and format characters, scrubs 32-hex path ids (case-insensitive, any depth), and caps output by code point. JSON.stringify(res.errors) is gone from every script. - Every scope name operators see uses the dashboard's arrow notation (Zone → WAF: Edit), guarded by a mutation-verified drift test, and the token recipe gained the missing Zone → Zone: Read entry. - The scripts/ suite grows 213 → 260, including branch-discriminating no-leak sweeps and source-contract pins for the warn sites inside unimportable main() functions. --- README.md | 24 +- UPDATING.md | 2 +- scripts/apply-download-ratelimit.ts | 15 +- scripts/connect-domains-lib.test.ts | 6 +- scripts/connect-domains-lib.ts | 20 +- scripts/connect-domains.test.ts | 8 + scripts/connect-domains.ts | 34 ++- scripts/setup-lib.test.ts | 393 +++++++++++++++++++++++++--- scripts/setup-lib.ts | 128 +++++++-- scripts/setup.ts | 66 +++-- scripts/turnstile-lib.test.ts | 223 +++++++++++++++- scripts/turnstile-lib.ts | 33 ++- scripts/typecheck.test.ts | 5 +- scripts/waf-lib.test.ts | 311 ++++++++++++++++++++-- scripts/waf-lib.ts | 46 +++- 15 files changed, 1162 insertions(+), 152 deletions(-) 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..056e7fe9 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -95,7 +95,7 @@ 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 +**Zone → WAF: Edit**, 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.ts b/scripts/apply-download-ratelimit.ts index 6f7239d9..cf5b869f 100644 --- a/scripts/apply-download-ratelimit.ts +++ b/scripts/apply-download-ratelimit.ts @@ -19,12 +19,12 @@ * zone, but writing the rule needs WAF: Edit.) */ import { env, argv, exit } from 'node:process'; -import { applyDownloadRateLimit } from './waf-lib.ts'; +import { applyDownloadRateLimit, isPermissionError } from './waf-lib.ts'; 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 → WAF: Edit\n' + ' and a Zone Resource that includes the fork domain, then re-run:\n' + ' CLOUDFLARE_API_TOKEN= npm run apply-download-ratelimit -- '; @@ -58,7 +58,16 @@ async function main(): Promise { return 0; default: console.error(`✖ ${res.detail}\n`); - console.error(TOKEN_RECIPE); + // The recipe fixes token scopes, so print it only when the detail names + // a permission failure (isPermissionError keys off waf-lib's own scope + // hint) — a transient HTTP error just gets plain retry guidance. + if (isPermissionError(res.detail)) { + console.error(TOKEN_RECIPE); + } else { + console.error('This may not be a token-permission problem. Re-run once the'); + console.error('reason above is resolved:'); + console.error(' CLOUDFLARE_API_TOKEN= npm run apply-download-ratelimit -- '); + } return 1; } } diff --git a/scripts/connect-domains-lib.test.ts b/scripts/connect-domains-lib.test.ts index 61906480..5a87bd75 100644 --- a/scripts/connect-domains-lib.test.ts +++ b/scripts/connect-domains-lib.test.ts @@ -464,7 +464,11 @@ describe('buildLadder + firstFailingRung', () => { expect(firstFailingRung(off)).toBeUndefined(); const unknown = buildLadder({ ...healthy, imageTransforms: null }); - expect(unknown.find((r) => r.id === 'image-transforms')?.status).toBe('warn'); + 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'); }); }); diff --git a/scripts/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index 613ed16e..db7e2302 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -73,11 +73,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<{ ok: boolean; status: number; result?: unknown; errors?: unknown }> ): 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 +90,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 + }; } /** @@ -361,7 +371,7 @@ 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 — the token lacks Account → Workers R2 Storage: Read (or a transient API error). 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 +390,7 @@ 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 (token lacks 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..a6261745 100644 --- a/scripts/connect-domains.test.ts +++ b/scripts/connect-domains.test.ts @@ -159,6 +159,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*\)/); }); diff --git a/scripts/connect-domains.ts b/scripts/connect-domains.ts index f84359d6..66bfbb4b 100644 --- a/scripts/connect-domains.ts +++ b/scripts/connect-domains.ts @@ -32,6 +32,7 @@ import { stdin, stdout, env, argv, exit } from 'node:process'; import { fileURLToPath } from 'node:url'; import { cfApi, + cfErrorSummary, hostFromDomain, zoneNameCandidates, imageResizingOutcome, @@ -58,11 +59,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 +149,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 +165,12 @@ async function main(): Promise { `✖ Could not reach the Cloudflare API while looking up the zone for ${lookupName} — check your network and re-run.` ); } else { + // A 2xx here means the API answered but its body said success:false — + // repeat the API's own reason when it gave one. + const apiWhy = + errorStatus >= 200 && errorStatus < 300 ? 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; @@ -193,7 +199,7 @@ async function main(): Promise { ); if (cdnState === 'unknown') 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 (token may lack Account → Workers R2 Storage: Read) — skipping the ${cdn} attach.` ); const plan = planConnect({ @@ -217,7 +223,7 @@ async function main(): Promise { console.log( transformsCurrent === true ? ' Image Transformations: on.' - : " Image Transformations: couldn't verify (token lacks Zone Settings·Read)." + : " Image Transformations: couldn't verify (token lacks Zone → Zone Settings: Read)." ); console.log(` Re-check anytime: npm run connect-domains -- --check ${host}`); return 0; @@ -258,7 +264,11 @@ 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 { + // Allowlisted code+message pairs only — never the raw errors body. + const why = cfErrorSummary(res.errors); + console.warn(`⚠ Could not ${m.label} (HTTP ${res.status})${why ? ` ${why}` : ''}`); + } } if (willEnableTransforms) { @@ -275,7 +285,7 @@ async function main(): Promise { 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 (token lacks Zone → Zone Settings: Read) — enable it in the dashboard." ); } diff --git a/scripts/setup-lib.test.ts b/scripts/setup-lib.test.ts index 2d7154e9..9efbea7c 100644 --- a/scripts/setup-lib.test.ts +++ b/scripts/setup-lib.test.ts @@ -21,10 +21,15 @@ import { imageResizingIsOn, ciWiringEntries, cfApi, + cfErrorSummary, securitySummaryLines, 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', () => { @@ -611,47 +616,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 access to the taro.surf zone'); + 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 +749,25 @@ 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([]); }); }); @@ -695,13 +779,138 @@ 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 via cfErrorSummary', () => { + // main()s are not importable, so pin the wiring at source level: the + // resolveZone consumers must thread the errors body into cfErrorSummary. + expect(src).toContain('cfErrorSummary(zoneLookupErrors)'); + const connectSrc = readFileSync( + join(dirname(fileURLToPath(import.meta.url)), 'connect-domains.ts'), + 'utf8' + ); + expect(connectSrc).toContain('cfErrorSummary(errors)'); + }); + + it('setup’s zone-lookup warn attributes the API reason, not a bare parenthetical', () => { + // Three sites print '; the API said …' (this warn, connect-domains' error + // line, and cfFailureTail); pin this inline copy against drift. + expect(src).toContain('; the API said ${apiWhy}'); + expect(src).not.toContain('(${apiWhy})'); + }); + + 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 cfErrorSummary. + 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(source).toMatch(/cfErrorSummary\(res\.errors\)/); + // The empty-why case must not leave a trailing space after the status — + // pin the exact spacing ternary (a plain `) ${why}` mutant survives tests). + expect(source).toContain("${why ? ` ${why}` : ''}"); + } + }); + + 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 (·) scope name reintroduced in an operator-facing + // surface. Code comments are out of scope; connect-domains-lib's '·' + // skip-glyph is the one allowed code occurrence. + 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('·'); + + // The recipes' positive pins, and apply-download-ratelimit's negative one + // (its file is not in the whole-file scan below, which subsumes the rest). + const recipeOf = (file: string) => { + const source = readFileSync(join(here, file), 'utf8'); + const recipe = source.match(/const TOKEN_RECIPE =[\s\S]*?';/)?.[0] ?? ''; + expect(recipe, `${file} TOKEN_RECIPE found`).not.toBe(''); + return recipe; + }; + expect(recipeOf('apply-download-ratelimit.ts')).not.toContain('·'); + expect(recipeOf('setup.ts')).toContain('Zone → Zone: Read'); + expect(recipeOf('setup.ts')).toContain('needed later to attach the apex record'); + + // Whole-file scan of the CLIs' non-comment code (printed strings live + // there): drop full-line and trailing // comments (a URL's :// never + // matches — the strip needs whitespace or line start before //), minus + // the one legal skip-glyph. + for (const file of ['setup.ts', 'connect-domains.ts', 'connect-domains-lib.ts']) { + const code = readFileSync(join(here, file), 'utf8') + .split('\n') + .filter((l) => !/^\s*(\/\/|\*|\/\*)/.test(l)) + .map((l) => l.replace(/(^|\s)\/\/.*$/, '')) + .filter((l) => !l.includes("skip: '·'")) + .join('\n'); + expect(code, `${file} non-comment code`).not.toContain('·'); + } + + // The two connect-domains console strings hold the arrow form (positive + // pins so a wording rewrite can't silently drop the scope name). + const connectSource = readFileSync(join(here, 'connect-domains.ts'), 'utf8'); + expect(connectSource).toContain( + "Image Transformations: couldn't verify (token lacks Zone → Zone Settings: Read)." + ); + expect(connectSource).toContain( + "Image Transformations: couldn't verify (token lacks Zone → Zone Settings: Read) — enable it in the dashboard." + ); + + // 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', () => { @@ -714,6 +923,126 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { }); }); +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('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); + }); +}); + describe('pagesPatchConfirmsSitekey', () => { const body = (value?: string) => ({ deployment_configs: { diff --git a/scripts/setup-lib.ts b/scripts/setup-lib.ts index b72bd3bc..1d0ab0c0 100644 --- a/scripts/setup-lib.ts +++ b/scripts/setup-lib.ts @@ -315,6 +315,83 @@ export async function cfApi( } } +// 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 32-hex path segment in the message — + * case-insensitive, however deep — is scrubbed to ``: Cloudflare's own + * text can echo object ids (code 7003 quotes the request path, zone id + * included). 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((e): e is { code?: unknown; message?: unknown } => typeof e === 'object' && e !== null) + .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. + const raw = + typeof e.message === 'string' + ? e.message + .replace(/[\p{Cc}\p{Cf}]/gu, ' ') + .replace(/\s+/g, ' ') + .trim() + .replace(/(?<=\/)[0-9a-f]{32}(?=[/,.\s]|$)/gi, '') + : ''; + 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})`; +} + export interface GhEligibilityInput { /** `gh` binary is on PATH. */ ghInstalled: boolean; @@ -401,34 +478,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 +525,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,7 +539,7 @@ 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; diff --git a/scripts/setup.ts b/scripts/setup.ts index 4d420804..a549dd0d 100644 --- a/scripts/setup.ts +++ b/scripts/setup.ts @@ -41,13 +41,18 @@ import { imageResizingIsOn, ciWiringEntries, cfApi, + cfErrorSummary, securitySummaryLines, 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'; @@ -126,13 +131,14 @@ const token = (bytes = 32) => randomBytes(bytes).toString('hex'); // 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)'; + ' • Account → Cloudflare Pages: Edit\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'); @@ -322,8 +328,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 +356,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,17 +364,21 @@ 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 ". + // status 0 = fetch threw (no network). A 2xx means the API answered + // but its body said success:false, so repeat the API's own reason. + const apiWhy = + zoneLookupError >= 200 && zoneLookupError < 300 + ? cfErrorSummary(zoneLookupErrors) + : ''; const why = zoneLookupError === 0 ? 'could not reach the Cloudflare API' - : `Cloudflare API error, HTTP ${zoneLookupError}`; + : `Cloudflare API error, HTTP ${zoneLookupError}${apiWhy ? `; the API said ${apiWhy}` : ''}`; 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.' + ' A 401/403 means the token lacks Zone → Zone: Read; otherwise re-run setup to retry.' ); } else if (!zoneId) { console.warn( @@ -376,7 +393,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.'); @@ -407,7 +424,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 +440,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 +457,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).'); } } @@ -509,7 +527,10 @@ async function main() { ); } else { console.warn('\n⚠ Could not attach bindings to the Pages project via the API'); - console.warn(` (HTTP ${res.status}) ${JSON.stringify(res.errors ?? '')}`); + // Only the allowlisted code+message pairs — never the raw errors body, + // which can echo account/project identifiers into pasteable output. + const why = cfErrorSummary(res.errors); + console.warn(` (HTTP ${res.status})${why ? ` ${why}` : ''}`); 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.'); @@ -701,14 +722,15 @@ 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); } } diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index c41f9249..1af55951 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -41,6 +41,8 @@ function fakeApi(routes: Record) { const listPath = `GET /accounts/${ACCT}/challenges/widgets?per_page=50`; 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 +96,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 } } }); @@ -138,10 +140,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 } } }); @@ -170,7 +169,7 @@ describe('provisionTurnstileWidget — reuses when present (idempotent)', () => [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,6 +178,11 @@ 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); }); }); @@ -190,13 +194,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 +248,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 +377,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 +390,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..a2697f92 100644 --- a/scripts/turnstile-lib.ts +++ b/scripts/turnstile-lib.ts @@ -19,7 +19,7 @@ * 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, 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 +43,14 @@ 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'; + +/** 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 { @@ -76,8 +82,8 @@ export interface TurnstileResult { * * 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. + * non-ok response → a clear error whose detail carries the actual reason + * (the scope hint on 401/403, otherwise just the HTTP status). 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 @@ -110,7 +116,7 @@ export async function provisionTurnstileWidget( if (!listRes.ok) { return { status: 'error', - detail: `could not list Turnstile widgets (HTTP ${listRes.status}); token needs ${SCOPE_HINT}` + detail: `could not list Turnstile widgets${statusLabel(listRes.status)}${failureTail(listRes)}` }; } const widgets = (listRes.result as Widget[] | undefined) ?? []; @@ -124,9 +130,14 @@ export async function provisionTurnstileWidget( const getRes = await api(cfToken, `/accounts/${accountId}/challenges/widgets/${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 +155,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..744bffc4 100644 --- a/scripts/waf-lib.test.ts +++ b/scripts/waf-lib.test.ts @@ -3,6 +3,7 @@ import type { CfApiResult } from './setup-lib.ts'; import { applyDownloadRateLimit, buildRule, + isPermissionError, RULE_REF, RULE_DESCRIPTION, RULE_EXPRESSION, @@ -41,6 +42,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 +81,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'); @@ -243,7 +245,7 @@ describe('applyDownloadRateLimit — clear errors, no mutation', () => { }); 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('no access to the akito.dog zone'); expect(res.detail).toContain('WAF: Edit'); // Never proceeded to the ruleset endpoint. expect(calls).toHaveLength(1); @@ -267,6 +269,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 +285,10 @@ 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. + expect(res.detail).toContain('token needs'); + expect(res.detail).toContain('WAF: Edit'); expect(calls).toHaveLength(1); }); @@ -296,6 +304,71 @@ 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'); + }); + it('empty domain → error before any network call', async () => { const { api, calls } = fakeApi({}); const res = await applyDownloadRateLimit(SECRET, ' ', api); @@ -305,39 +378,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 +543,96 @@ 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 }, + [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 }, - [`PUT /zones/${ZONE}/rulesets/phases/http_ratelimit/entrypoint`]: { ok: false, status: 500 } + [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.detail)).toBe(true); + + const noAccess = await applyDownloadRateLimit( + SECRET, + 'akito.dog', + fakeApi({ [zonePath]: { ok: true, status: 200, result: [] } }).api + ); + expect(isPermissionError(noAccess.detail)).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.detail)).toBe(false); }); }); diff --git a/scripts/waf-lib.ts b/scripts/waf-lib.ts index 9a060b55..3fa963bd 100644 --- a/scripts/waf-lib.ts +++ b/scripts/waf-lib.ts @@ -12,7 +12,14 @@ * 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, + cfFailureTail, + hostFromDomain, + statusLabel, + zoneNameCandidates, + type CfApiResult +} from './setup-lib.ts'; import { resolveZone } from './connect-domains-lib.ts'; /** @@ -125,8 +132,25 @@ export interface RateLimitResult { detail: string; } -/** 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)'; +/** 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)'; + +/** setup-lib's shared cfFailureTail, bound to this lib's scope hint. */ +function failureTail(status: number, errors?: unknown): string { + return cfFailureTail(status, errors, SCOPE_HINT); +} + +/** + * True when an error `detail` names a token-permission failure. Every branch + * that blames permissions interpolates SCOPE_HINT verbatim (via failureTail's + * 401/403 arm or the no-zone-access detail) and no other branch mentions it, + * so the standalone runner can decide whether the token recipe is relevant + * without RateLimitResult carrying an HTTP status. + */ +export function isPermissionError(detail: string): boolean { + return detail.includes(SCOPE_HINT); +} /** * Idempotently apply the public-endpoint rate-limit rule to `domain`'s zone. @@ -164,13 +188,14 @@ 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) { 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}${statusLabel(errorStatus)}${failureTail(errorStatus, errors)}` }; } zoneId = zone.id; @@ -178,7 +203,7 @@ export async function applyDownloadRateLimit( if (!zoneId) { return { status: 'error', - detail: `token has no access to zone ${host}: add ${SCOPE_HINT}` + detail: `the token has no access to the ${host} zone; add ${SCOPE_HINT}` }; } @@ -194,7 +219,7 @@ export async function applyDownloadRateLimit( // 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}${statusLabel(entry.status)}${failureTail(entry.status, entry.errors)}` }; } @@ -229,9 +254,12 @@ export async function applyDownloadRateLimit( })(); 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}${statusLabel(write.status)}${failureTail(write.status, write.errors)}` }; } return mine From 794377a623147ce942e195bd6d618595f4c026ed Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:19:57 -0700 Subject: [PATCH 02/16] fix(setup): page through the Turnstile widget list when matching ours provisionTurnstileWidget read only the first page of the account's widgets, so on an account holding more than 50 the name+host match could miss ours. A miss is not free: the next setup run mints a duplicate widget and rewires the Pages project to its sitekey and secret. Walk the list with page/per_page until the widget turns up, a page comes back short, or a page cap is hit. A non-ok page returns the same error detail a first-page failure already returned. --- scripts/turnstile-lib.test.ts | 62 ++++++++++++++++++++++++++++++++++- scripts/turnstile-lib.ts | 54 ++++++++++++++++++++---------- 2 files changed, 97 insertions(+), 19 deletions(-) diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index 1af55951..548aac36 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -38,7 +38,17 @@ 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}`; +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. @@ -187,6 +197,56 @@ describe('provisionTurnstileWidget — reuses when present (idempotent)', () => }); }); +// 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); + }); + + 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); + }); + + 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); + }); +}); + describe('provisionTurnstileWidget — clear errors, no mutation', () => { it('token lacks Turnstile scope (list 403) → error naming the scope, no create', async () => { const { api, calls } = fakeApi({ diff --git a/scripts/turnstile-lib.ts b/scripts/turnstile-lib.ts index a2697f92..7a987534 100644 --- a/scripts/turnstile-lib.ts +++ b/scripts/turnstile-lib.ts @@ -47,6 +47,17 @@ export const WIDGET_MODE = 'managed'; * 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); @@ -81,8 +92,9 @@ 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 → a clear error whose detail carries the actual reason + * 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). 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: @@ -96,11 +108,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, @@ -111,18 +123,24 @@ 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${statusLabel(listRes.status)}${failureTail(listRes)}` - }; + // 1. List existing widgets a page at a time; reconcile against ours by stable + // name. Stop at the first match, at a short page (the last one), or at MAX_PAGES. + 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}` + ); + if (!listRes.ok) { + return { + status: 'error', + detail: `could not list Turnstile widgets${statusLabel(listRes.status)}${failureTail(listRes)}` + }; + } + const widgets = (listRes.result as Widget[] | undefined) ?? []; + mine = widgets.find((w) => w.name === WIDGET_NAME && w.sitekey && w.domains?.includes(host)); + if (mine || widgets.length < PER_PAGE) break; } - 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). From 6c0ac97b32a1eb4165b0a19c0d9824ab05805f5d Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:33:01 -0700 Subject: [PATCH 03/16] fix(setup): stable ordering for the widget-list walk, and pin the page cap The offset walk sorts by created_on ascending so a widget deleted between page reads can't shift ours onto a page already read, and MAX_PAGES gets the test the mutation run showed it lacked: an API that ignores 'page' now provably stops after 20 GETs instead of hanging setup. --- scripts/turnstile-lib.test.ts | 34 +++++++++++++++++++++++++++++++++- scripts/turnstile-lib.ts | 5 ++++- 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index 548aac36..a8924ddc 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -40,7 +40,7 @@ function fakeApi(routes: Record) { const PER_PAGE = 50; const listPage = (page: number) => - `GET /accounts/${ACCT}/challenges/widgets?page=${page}&per_page=${PER_PAGE}`; + `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) => @@ -215,6 +215,8 @@ describe('provisionTurnstileWidget — walks past the first list page', () => { 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 () => { @@ -245,6 +247,36 @@ describe('provisionTurnstileWidget — walks past the first list page', () => { // 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', 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 }); + 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); + // Current behavior, asserted so a change to it is a deliberate one: the walk + // ending without a match is indistinguishable from "we have no widget", so the + // create fires. It mints a duplicate on an account this large, which beats + // leaving the fork with no widget and a locked admin login. + expect(res.status).toBe('created'); + expect(calls.filter((c) => c.method === 'POST')).toHaveLength(1); + }); }); describe('provisionTurnstileWidget — clear errors, no mutation', () => { diff --git a/scripts/turnstile-lib.ts b/scripts/turnstile-lib.ts index 7a987534..2e5a271a 100644 --- a/scripts/turnstile-lib.ts +++ b/scripts/turnstile-lib.ts @@ -125,11 +125,14 @@ export async function provisionTurnstileWidget( // 1. List existing widgets a page at a time; reconcile against ours by stable // name. Stop at the first match, at a short page (the last one), or at MAX_PAGES. + // The explicit sort keeps the page offsets stable: under the API's default order a + // widget deleted mid-walk shifts later widgets back a slot, and ours could slide + // from an unread page onto one already read. 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}` + `/accounts/${accountId}/challenges/widgets?page=${page}&per_page=${PER_PAGE}&order=created_on&direction=asc` ); if (!listRes.ok) { return { From 13e9b3351e338aafe1defa3c185cf685210bd76d Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:44:02 -0700 Subject: [PATCH 04/16] polish(setup): honest sort-guarantee comment, fast-fail guard in the page-cap test The sort comment claimed deletion-proof offsets; it now says what created_on ascending actually buys (a fixed ordering for the walk's lifetime) and that a concurrent delete still costs at most a duplicate. The exhaustion test's fake api now throws past 25 GETs, so removing the page bound fails in milliseconds with a named error instead of a heap OOM that voids the whole file's results. --- scripts/turnstile-lib.test.ts | 6 ++++++ scripts/turnstile-lib.ts | 7 ++++--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index a8924ddc..65915acf 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -260,6 +260,12 @@ describe('provisionTurnstileWidget — walks past the first list page', () => { ): 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 } }; } diff --git a/scripts/turnstile-lib.ts b/scripts/turnstile-lib.ts index 2e5a271a..9e17c756 100644 --- a/scripts/turnstile-lib.ts +++ b/scripts/turnstile-lib.ts @@ -125,9 +125,10 @@ export async function provisionTurnstileWidget( // 1. List existing widgets a page at a time; reconcile against ours by stable // name. Stop at the first match, at a short page (the last one), or at MAX_PAGES. - // The explicit sort keeps the page offsets stable: under the API's default order a - // widget deleted mid-walk shifts later widgets back a slot, and ours could slide - // from an unread page onto one already read. + // 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( From d416520a2b632cc2850db977a926701d69fb6472 Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 14:04:03 -0700 Subject: [PATCH 05/16] fix(setup): report each Cloudflare failure by its own status connect-domains recommended the R2 read scope for every failed bucket read, including a 5xx and an unreachable API, and dropped the API's own reason on any zone lookup that wasn't a 2xx. Route the attach, transforms, and bucket failures through cfFailureTail/statusLabel so a scope is named only on 401/403, a thrown fetch says the API did not respond instead of "HTTP 0", and the sanitized reason is repeated whatever the status. The Turnstile widget walk read a non-array list body as an empty page, which ended the walk and minted a duplicate widget; it now errors on a partial body. resolveZone's lookup takes the shared CfApiResult type so the contract cannot drift. --- scripts/connect-domains-lib.test.ts | 8 ++++++ scripts/connect-domains-lib.ts | 15 ++++++++--- scripts/connect-domains.test.ts | 41 +++++++++++++++++++++++++++++ scripts/connect-domains.ts | 39 +++++++++++++++++++-------- scripts/setup-lib.test.ts | 12 ++++++--- scripts/turnstile-lib.test.ts | 28 ++++++++++++++++++++ scripts/turnstile-lib.ts | 14 ++++++++-- 7 files changed, 136 insertions(+), 21 deletions(-) diff --git a/scripts/connect-domains-lib.test.ts b/scripts/connect-domains-lib.test.ts index 5a87bd75..0cfd061b 100644 --- a/scripts/connect-domains-lib.test.ts +++ b/scripts/connect-domains-lib.test.ts @@ -313,6 +313,14 @@ describe('planConnect', () => { expect(planConnect({ ...base, cdnPresent: true, pagesAttached: 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, pagesAttached: 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', () => { const state = cdnDomainState( { ok: true, status: 200, result: { domains: [{ domain: 'cdn.taro.surf', enabled: false }] } }, diff --git a/scripts/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index db7e2302..c13ad90b 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -6,8 +6,11 @@ * 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. The one exception is a type-only import of that + * module's `CfApiResult`, so the shapes this file consumes can't drift from + * what `cfApi` actually returns; a type import adds no runtime dependency. */ +import type { CfApiResult } from './setup-lib.ts'; /** `cdn.` — the CDN subdomain we attach to the images R2 bucket. */ export function cdnHost(host: string): string { @@ -73,7 +76,7 @@ export function classifyZone(result: unknown): ZoneStatus { */ export async function resolveZone( candidates: string[], - lookup: (name: string) => Promise<{ ok: boolean; status: number; result?: unknown; errors?: unknown }> + lookup: (name: string) => Promise ): Promise<{ zone: ZoneStatus; zoneName: string | null; @@ -250,6 +253,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; } /** @@ -268,14 +273,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) 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; } diff --git a/scripts/connect-domains.test.ts b/scripts/connect-domains.test.ts index a6261745..bdc395f6 100644 --- a/scripts/connect-domains.test.ts +++ b/scripts/connect-domains.test.ts @@ -184,4 +184,45 @@ 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);'); + expect(src).not.toContain('errorStatus >= 200 && errorStatus < 300'); + }); +}); + +// Every other failed call in the mutating path reports through cfFailureTail, 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' + ); + + it('reports the R2 custom-domain read through cfFailureTail', () => { + expect(src).toMatch(/cfFailureTail\(\s*r2Res\.status,\s*r2Res\.errors,\s*'Account → Workers R2 Storage: Read'/); + expect(src).not.toContain('token may lack Account → Workers R2 Storage: Read'); + }); + + it("reports a failed attach with that mutation's own scope, never a bare HTTP 0", () => { + expect(src).toContain('cfFailureTail(res.status, res.errors, m.scopeHint)'); + expect(src).not.toContain('(HTTP ${res.status})'); + }); + + it('reports a failed Image Transformations enable the same way', () => { + expect(src).toMatch(/cfFailureTail\(\s*patched\.status,\s*patched\.errors,\s*'Zone → Zone Settings: Edit'/); + expect(src).not.toContain('(HTTP ${patched.status})'); + }); + + it('uses statusLabel so a thrown fetch prints no status at all', () => { + expect(src).toContain('statusLabel(r2Res.status)'); + expect(src).toContain('statusLabel(res.status)'); + expect(src).toContain('statusLabel(patched.status)'); + }); }); diff --git a/scripts/connect-domains.ts b/scripts/connect-domains.ts index 66bfbb4b..112c3750 100644 --- a/scripts/connect-domains.ts +++ b/scripts/connect-domains.ts @@ -33,6 +33,8 @@ import { fileURLToPath } from 'node:url'; import { cfApi, cfErrorSummary, + cfFailureTail, + statusLabel, hostFromDomain, zoneNameCandidates, imageResizingOutcome, @@ -165,10 +167,11 @@ async function main(): Promise { `✖ Could not reach the Cloudflare API while looking up the zone for ${lookupName} — check your network and re-run.` ); } else { - // A 2xx here means the API answered but its body said success:false — - // repeat the API's own reason when it gave one. - const apiWhy = - errorStatus >= 200 && errorStatus < 300 ? cfErrorSummary(errors) : ''; + // 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}${apiWhy ? `; the API said ${apiWhy}` : ''}) while looking up the zone for ${lookupName} — wait a moment and re-run.` ); @@ -198,8 +201,15 @@ async function main(): Promise { `⚠ ${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${statusLabel(r2Res.status)}${cfFailureTail( + r2Res.status, + r2Res.errors, + 'Account → Workers R2 Storage: Read' + )} — skipping the ${cdn} attach.` ); const plan = planConnect({ @@ -264,11 +274,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 { - // Allowlisted code+message pairs only — never the raw errors body. - const why = cfErrorSummary(res.errors); - console.warn(`⚠ Could not ${m.label} (HTTP ${res.status})${why ? ` ${why}` : ''}`); - } + else + // cfFailureTail 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}${statusLabel(res.status)}${cfFailureTail(res.status, res.errors, m.scopeHint)}` + ); } if (willEnableTransforms) { @@ -279,7 +292,11 @@ 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${statusLabel(patched.status)}${cfFailureTail( + patched.status, + patched.errors, + 'Zone → Zone Settings: Edit' + )} — enable it in the dashboard.` ); } else if (transformsCurrent === true) { console.log('✔ Image Transformations already on.'); diff --git a/scripts/setup-lib.test.ts b/scripts/setup-lib.test.ts index 9efbea7c..f231ab1f 100644 --- a/scripts/setup-lib.test.ts +++ b/scripts/setup-lib.test.ts @@ -834,11 +834,15 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { ); for (const source of [src, connectSrc]) { expect(source).not.toMatch(/JSON\.stringify\(res\.errors/); - expect(source).toMatch(/cfErrorSummary\(res\.errors\)/); - // The empty-why case must not leave a trailing space after the status — - // pin the exact spacing ternary (a plain `) ${why}` mutant survives tests). - expect(source).toContain("${why ? ` ${why}` : ''}"); } + expect(src).toMatch(/cfErrorSummary\(res\.errors\)/); + // The empty-why case must not leave a trailing space after the status — + // pin the exact spacing ternary (a plain `) ${why}` mutant survives tests). + expect(src).toContain("${why ? ` ${why}` : ''}"); + // connect-domains' attach failures report through cfFailureTail, which + // sanitizes through the same cfErrorSummary and punctuates the reason + // itself — so the status also decides whether a scope is named at all. + expect(connectSrc).toContain('cfFailureTail(res.status, res.errors, m.scopeHint)'); }); it('no middot scope names remain anywhere an operator sees one', () => { diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index 65915acf..9742094d 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -234,6 +234,34 @@ describe('provisionTurnstileWidget — walks past the first list page', () => { 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') }, diff --git a/scripts/turnstile-lib.ts b/scripts/turnstile-lib.ts index 9e17c756..588ea20a 100644 --- a/scripts/turnstile-lib.ts +++ b/scripts/turnstile-lib.ts @@ -95,7 +95,8 @@ export interface TurnstileResult { * 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). No mutation. + * (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 @@ -141,7 +142,16 @@ export async function provisionTurnstileWidget( detail: `could not list Turnstile widgets${statusLabel(listRes.status)}${failureTail(listRes)}` }; } - const widgets = (listRes.result as Widget[] | undefined) ?? []; + // An ok page whose result isn't a list 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. + if (!Array.isArray(listRes.result)) { + 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[]; mine = widgets.find((w) => w.name === WIDGET_NAME && w.sitekey && w.domains?.includes(host)); if (mine || widgets.length < PER_PAGE) break; } From a1ecd6e0fff667089329bc2ca4271a66c7bfaca7 Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:57:19 -0700 Subject: [PATCH 06/16] fix(setup): scrub ids anywhere, and never create on an unfinished widget walk --- scripts/setup-lib.test.ts | 22 +++++++++++++++++++++ scripts/setup-lib.ts | 12 +++++++----- scripts/turnstile-lib.test.ts | 37 ++++++++++++++++++++++++++++++----- scripts/turnstile-lib.ts | 31 +++++++++++++++++++++++------ 4 files changed, 86 insertions(+), 16 deletions(-) diff --git a/scripts/setup-lib.test.ts b/scripts/setup-lib.test.ts index f231ab1f..7e746b41 100644 --- a/scripts/setup-lib.test.ts +++ b/scripts/setup-lib.test.ts @@ -1004,6 +1004,28 @@ describe('cfErrorSummary', () => { 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'); + }); + + 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'); diff --git a/scripts/setup-lib.ts b/scripts/setup-lib.ts index 1d0ab0c0..e8b07441 100644 --- a/scripts/setup-lib.ts +++ b/scripts/setup-lib.ts @@ -325,10 +325,12 @@ const capPoints = (value: string, max: number): string => { /** * 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 32-hex path segment in the message — - * case-insensitive, however deep — is scrubbed to ``: Cloudflare's own - * text can echo object ids (code 7003 quotes the request path, zone id - * included). The message content is otherwise printed verbatim. Anything + * 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). */ @@ -347,7 +349,7 @@ export function cfErrorSummary(errors: unknown): string { .replace(/[\p{Cc}\p{Cf}]/gu, ' ') .replace(/\s+/g, ' ') .trim() - .replace(/(?<=\/)[0-9a-f]{32}(?=[/,.\s]|$)/gi, '') + .replace(/\b[0-9a-f]{32}\b/gi, '') : ''; if (!raw) return ''; const message = capPoints(raw, 200); diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index 9742094d..370085a3 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -279,7 +279,7 @@ describe('provisionTurnstileWidget — walks past the first list page', () => { // 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', async () => { + it('stops after MAX_PAGES when every page comes back full, without creating', async () => { const calls: Call[] = []; const api = async ( token: string, @@ -304,10 +304,37 @@ describe('provisionTurnstileWidget — walks past the first list page', () => { // Terminated at the bound instead of hanging. expect(calls.filter((c) => c.method === 'GET')).toHaveLength(20); - // Current behavior, asserted so a change to it is a deliberate one: the walk - // ending without a match is indistinguishable from "we have no widget", so the - // create fires. It mints a duplicate on an account this large, which beats - // leaving the fork with no widget and a locked admin login. + // 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); }); diff --git a/scripts/turnstile-lib.ts b/scripts/turnstile-lib.ts index 588ea20a..f065a692 100644 --- a/scripts/turnstile-lib.ts +++ b/scripts/turnstile-lib.ts @@ -125,7 +125,8 @@ export async function provisionTurnstileWidget( if (!host) return { status: 'error', detail: 'no domain given' }; // 1. List existing widgets a page at a time; reconcile against ours by stable - // name. Stop at the first match, at a short page (the last one), or at MAX_PAGES. + // 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 @@ -142,18 +143,36 @@ export async function provisionTurnstileWidget( detail: `could not list Turnstile widgets${statusLabel(listRes.status)}${failureTail(listRes)}` }; } - // An ok page whose result isn't a list 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. - if (!Array.isArray(listRes.result)) { + // 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[]; - mine = widgets.find((w) => w.name === WIDGET_NAME && w.sitekey && w.domains?.includes(host)); + mine = widgets.find( + (w) => + w.name === WIDGET_NAME && w.sitekey && Array.isArray(w.domains) && w.domains.includes(host) + ); 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` + }; + } } // 2a. Reuse: fetch the single widget so we read its secret from the authoritative From 751289082020a3ebf1d8a1274535fd28254e313f Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:00:41 -0700 Subject: [PATCH 07/16] fix(setup): treat a sitekey-less entry of ours as found, not missing A widget carrying our name and our host is ours even when its sitekey comes back missing or non-string. Folding the sitekey test into the match read such an entry as somebody else's and walked on to create, minting a second widget for the same name and host. The match now keys on name plus host, and an unusable sitekey stops the run instead. --- scripts/turnstile-lib.test.ts | 20 ++++++++++++++++++++ scripts/turnstile-lib.ts | 16 +++++++++++++--- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index 370085a3..9832ad29 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -174,6 +174,26 @@ 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]: { diff --git a/scripts/turnstile-lib.ts b/scripts/turnstile-lib.ts index f065a692..93f30556 100644 --- a/scripts/turnstile-lib.ts +++ b/scripts/turnstile-lib.ts @@ -158,10 +158,20 @@ export async function provisionTurnstileWidget( }; } const widgets = listRes.result as Widget[]; - mine = widgets.find( - (w) => - w.name === WIDGET_NAME && w.sitekey && Array.isArray(w.domains) && w.domains.includes(host) + // 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 From 9982cd0b2da5e3a374536bb83bddc7f1eabe80ba Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:17:00 -0700 Subject: [PATCH 08/16] fix(connect-domains): read the failure reason off the response, and never attach on an unread state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sweep the whole CLI for the two classes three review rounds each found in a new spot. Scope blame regardless of status: the Image Transformations "couldn't verify" lines (both the already-connected summary and the post-mutation tail) and the doctor's cdn-attached and image-transforms rungs all named a token permission as the cause for every failure, including a 5xx, an unreachable API, and a 2xx-with-success:false. Each now routes through cfFailureTail + statusLabel, so the scope appears only on a 401/403 and the API's own reason appears when it gave one. The doctor threads each read's status and errors into the ladder, so imageResizingOutcome() === null no longer collapses several causes into one unexplained "unknown". Mutating on an unproven state: a failed Pages-domain read produced an empty list, which read as "not attached" and sent the attach out anyway — the read was what would have told us whether the attach was needed. pagesDomainState mirrors cdnDomainState's auth-vs-absent distinction, and an unknown state warns and skips the attach the way the R2 path already did. An empty plan built on a failed read no longer prints "Already connected". --- scripts/connect-domains-lib.test.ts | 93 ++++++++++++++++++++++++++--- scripts/connect-domains-lib.ts | 65 +++++++++++++++++--- scripts/connect-domains.test.ts | 75 +++++++++++++++++++++++ scripts/connect-domains.ts | 50 ++++++++++++++-- scripts/setup-lib.test.ts | 18 +++--- 5 files changed, 271 insertions(+), 30 deletions(-) diff --git a/scripts/connect-domains-lib.test.ts b/scripts/connect-domains-lib.test.ts index 0cfd061b..1a9d87b5 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', () => { @@ -289,7 +291,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,19 +306,19 @@ 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, pagesAttached: false }); + 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'); }); @@ -327,7 +329,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 }); }); @@ -471,13 +473,90 @@ describe('buildLadder + firstFailingRung', () => { expect(off.find((r) => r.id === 'cdn-loads')?.status).toBe('pass'); expect(firstFailingRung(off)).toBeUndefined(); - const unknown = buildLadder({ ...healthy, imageTransforms: null }); + 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;"); + }); + + it('reports the R2 read failure by status too, not always as a missing scope', () => { + const cdnAction = (over: Partial) => + buildLadder({ ...healthy, cdnState: 'unknown', tlsIssued: null, ...over }).find( + (r) => r.id === 'cdn-attached' + )!.action!; + expect(cdnAction({ cdnReadStatus: 403 })).toContain('token needs Account → Workers R2 Storage: Read'); + expect(cdnAction({ cdnReadStatus: 503 })).not.toContain('Account → Workers R2 Storage: Read'); + expect(cdnAction({ cdnReadStatus: 0 })).toContain('the Cloudflare API did not respond'); + }); +}); + +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'); + }); + + 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([]); + }); }); describe('renderLadder secret-free output', () => { diff --git a/scripts/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index c13ad90b..cf8320ed 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -6,11 +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. The one exception is a type-only import of that - * module's `CfApiResult`, so the shapes this file consumes can't drift from - * what `cfApi` actually returns; a type import adds no runtime dependency. + * 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 type { CfApiResult } from './setup-lib.ts'; +import { cfFailureTail, statusLabel, type CfApiResult } from './setup-lib.ts'; /** `cdn.` — the CDN subdomain we attach to the images R2 bucket. */ export function cdnHost(host: string): string { @@ -222,6 +223,22 @@ export function pagesDomainAttached(result: unknown, host: string): boolean { return list.some((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'; + return pagesDomainAttached(res.result, host) ? 'attached' : 'absent'; +} + /** * Classifies an HTTPS probe to `cdn./`: `ok` for any real response * (a 4xx for a blank key still proves the domain + TLS are wired), `unreachable` @@ -245,7 +262,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 { @@ -264,6 +282,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[] = []; @@ -276,7 +299,7 @@ export function planConnect(i: ConnectPlanInput): PlannedMutation[] { 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`, @@ -322,10 +345,16 @@ export interface LadderInputs { zoneActive: boolean; /** attached = healthy, absent = not there, disabled = present-but-off, unknown = R2 read failed. */ cdnState: CdnDomainState; + /** The R2 read's HTTP status + errors body, so an `unknown` rung can say WHY. */ + cdnReadStatus?: number; + cdnReadErrors?: 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; @@ -333,6 +362,16 @@ 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 ? '' : `${statusLabel(status)}${cfFailureTail(status, errors, 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 @@ -378,7 +417,11 @@ 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${readFailureTail( + i.cdnReadStatus, + i.cdnReadErrors, + '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.`; @@ -397,7 +440,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 → 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 bdc395f6..750f9cbf 100644 --- a/scripts/connect-domains.test.ts +++ b/scripts/connect-domains.test.ts @@ -129,6 +129,38 @@ describe('runDoctor', () => { expect(text).not.toContain('Add the root domain'); }); + // 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. @@ -225,4 +257,47 @@ describe('connect-domains.ts ↔ failure-reporting source contract', () => { expect(src).toContain('statusLabel(res.status)'); expect(src).toContain('statusLabel(patched.status)'); }); + + it('reports the Pages-domain read through cfFailureTail as well', () => { + expect(src).toMatch( + /cfFailureTail\(\s*pagesRes\.status,\s*pagesRes\.errors,\s*'Account → Cloudflare Pages: Read'/ + ); + expect(src).toContain('statusLabel(pagesRes.status)'); + }); + + it('reads the Image Transformations reason off the response, not off a fixed scope', () => { + expect(src).toMatch( + /cfFailureTail\(\s*irGet\.status,\s*irGet\.errors,\s*'Zone → Zone Settings: Read'/ + ); + expect(src).not.toContain('token lacks 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'"); + // The old wiring read the list off a possibly-failed response, so a 403 + // became "not attached" and the attach went out anyway. + expect(src).not.toContain('pagesDomainAttached(pagesRes.result, host)'); + }); + + 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(/const unread = cdnState === 'unknown' \? cdn : pagesState === 'unknown' \? host : null/); + }); }); diff --git a/scripts/connect-domains.ts b/scripts/connect-domains.ts index 112c3750..c4439f28 100644 --- a/scripts/connect-domains.ts +++ b/scripts/connect-domains.ts @@ -49,7 +49,7 @@ import { zoneConsentLabel, cdnDomainState, bucketDomainTlsIssued, - pagesDomainAttached, + pagesDomainState, classifyCdnProbe, planConnect, siteUrlMismatch, @@ -194,7 +194,7 @@ 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( @@ -212,6 +212,18 @@ async function main(): Promise { )} — 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${statusLabel(pagesRes.status)}${cfFailureTail( + pagesRes.status, + pagesRes.errors, + 'Account → Cloudflare Pages: Read' + )} — skipping the ${host} attach.` + ); + const plan = planConnect({ accountId: cfAccount, bucket, @@ -219,7 +231,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 @@ -229,11 +241,23 @@ 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 → Zone Settings: Read)." + : ` Image Transformations: couldn't verify${statusLabel(irGet.status)}${cfFailureTail( + irGet.status, + irGet.errors, + 'Zone → Zone Settings: Read' + )}.` ); console.log(` Re-check anytime: npm run connect-domains -- --check ${host}`); return 0; @@ -302,7 +326,11 @@ async function main(): Promise { console.log('✔ Image Transformations already on.'); } else { console.log( - "⚠ Image Transformations: couldn't verify (token lacks Zone → Zone Settings: Read) — enable it in the dashboard." + `⚠ Image Transformations: couldn't verify${statusLabel(irGet.status)}${cfFailureTail( + irGet.status, + irGet.errors, + 'Zone → Zone Settings: Read' + )} — enable it in the dashboard.` ); } @@ -362,13 +390,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 @@ -383,8 +417,12 @@ export async function runDoctor(a: DoctorArgs, deps: DoctorDeps = defaultDoctorD zoneName: a.zoneName, candidates: a.candidates, cdnState, + cdnReadStatus: r2Read?.status, + cdnReadErrors: r2Read?.errors, 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 7e746b41..e050f68f 100644 --- a/scripts/setup-lib.test.ts +++ b/scripts/setup-lib.test.ts @@ -882,15 +882,17 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { expect(code, `${file} non-comment code`).not.toContain('·'); } - // The two connect-domains console strings hold the arrow form (positive - // pins so a wording rewrite can't silently drop the scope name). + // The two connect-domains transforms strings now hand the scope to + // cfFailureTail, which prints it only on a 401/403, so pin the arrow form at + // those call sites (positive pins so a rewrite can't drop the scope name) + // and pin out the old sentence that blamed the scope for every failure. const connectSource = readFileSync(join(here, 'connect-domains.ts'), 'utf8'); - expect(connectSource).toContain( - "Image Transformations: couldn't verify (token lacks Zone → Zone Settings: Read)." - ); - expect(connectSource).toContain( - "Image Transformations: couldn't verify (token lacks Zone → Zone Settings: Read) — enable it in the dashboard." - ); + expect( + connectSource.match( + /cfFailureTail\(\s*irGet\.status,\s*irGet\.errors,\s*'Zone → Zone Settings: Read'/g + ) + ).toHaveLength(2); + expect(connectSource).not.toContain('token lacks Zone → Zone Settings: Read'); // README and UPDATING.md: every line outside code fences. for (const doc of ['README.md', 'UPDATING.md']) { From c38138517efe03da508be476d821bac8528dfcf3 Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:21:36 -0700 Subject: [PATCH 09/16] fix(connect-domains): an unreadable Pages body is unknown, not absent A 200 whose result isn't a list was coerced to an empty list, so the domain read reported 'absent' and green-lit the attach it exists to gate. The malformed body now reads the same as a failed request. --- scripts/connect-domains-lib.test.ts | 12 ++++++++++++ scripts/connect-domains-lib.ts | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/scripts/connect-domains-lib.test.ts b/scripts/connect-domains-lib.test.ts index 1a9d87b5..621a62e3 100644 --- a/scripts/connect-domains-lib.test.ts +++ b/scripts/connect-domains-lib.test.ts @@ -536,6 +536,18 @@ describe('pagesDomainState', () => { 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'); diff --git a/scripts/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index cf8320ed..a9071a08 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -236,6 +236,10 @@ export function pagesDomainState( 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'; } From 0e8230f0421cb20b4d1fca7e0979a4ba4e03b6ff Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Thu, 20 Aug 2026 13:52:55 -0700 Subject: [PATCH 10/16] fix(setup): report what the API said, and only what setup established MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setup.ts held the last of the class this branch fixes elsewhere. The zone-lookup warn read the API's reason only on a 2xx, so a 400/404/500 that carried one dropped it and the operator got a bare status plus a guess at a missing scope. The Pages-binding warn printed "(HTTP 0)" for a fetch that never reached Cloudflare. Both now compose through statusLabel + cfFailureTail, which names Zone → Zone: Read or Account → Cloudflare Pages: Edit only on a 401/403. The summary also asserted state no write had established: the SETUP_TOKEN, CRON_SECRET, UPLOADTHING_TOKEN and TELEGRAM_BOT_TOKEN puts and the site_settings seed all ran with their results discarded, then printed as done. Each is now reported from its own result. A failed `wrangler d1 create` with no pasted id stops setup instead of wiring wrangler.toml and the Pages project to an empty database_id. The compositions moved into pure helpers in setup-lib.ts (zoneLookupWarnLines, storageSummaryLines, telegramSummaryLine, setupTokenLines, provisioningNoteLine) so their arms are unit-tested directly rather than pinned as source text, which is why the class survived in this file. --- scripts/setup-lib.test.ts | 177 ++++++++++++++++++++++++++++++++++---- scripts/setup-lib.ts | 102 ++++++++++++++++++++++ scripts/setup.ts | 107 ++++++++++++++--------- 3 files changed, 327 insertions(+), 59 deletions(-) diff --git a/scripts/setup-lib.test.ts b/scripts/setup-lib.test.ts index e050f68f..273a2fa1 100644 --- a/scripts/setup-lib.test.ts +++ b/scripts/setup-lib.test.ts @@ -23,6 +23,11 @@ import { cfApi, cfErrorSummary, securitySummaryLines, + zoneLookupWarnLines, + storageSummaryLines, + telegramSummaryLine, + setupTokenLines, + provisioningNoteLine, pagesPatchConfirmsSitekey, cdnAttachmentLines, type CfApiResult, @@ -771,6 +776,133 @@ describe('securitySummaryLines', () => { }); }); +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, uploadThingTokenSet: false }) + ).toEqual(['Storage backend: Cloudflare R2 (set up).']); + const missing = storageSummaryLines({ + ...base, + provider: 'r2', + r2Missing: true, + 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'); + }); + + it('only calls UploadThing set up when the token secret actually landed', () => { + expect( + storageSummaryLines({ ...base, provider: 'uploadthing', r2Missing: false, uploadThingTokenSet: true }) + ).toEqual(['Storage backend: UploadThing (set up).']); + const unset = storageSummaryLines({ + ...base, + provider: 'uploadthing', + r2Missing: false, + uploadThingTokenSet: false + }).join('\n'); + expect(unset).toContain('NOT READY (the UPLOADTHING_TOKEN secret is not set)'); + expect(unset).toContain('--project-name taro'); + }); +}); + +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'); + }); +}); + describe('setup.ts ↔ securitySummaryLines call-site contract', () => { // main() is not importable (it drives live Cloudflare state), so pin the // wiring at the source level: turnstileWired must be composed from the real @@ -806,10 +938,12 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { } ); - it('both zone-lookup warn sites repeat the API reason via cfErrorSummary', () => { + 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 cfErrorSummary. - expect(src).toContain('cfErrorSummary(zoneLookupErrors)'); + // 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' @@ -817,17 +951,11 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { expect(connectSrc).toContain('cfErrorSummary(errors)'); }); - it('setup’s zone-lookup warn attributes the API reason, not a bare parenthetical', () => { - // Three sites print '; the API said …' (this warn, connect-domains' error - // line, and cfFailureTail); pin this inline copy against drift. - expect(src).toContain('; the API said ${apiWhy}'); - expect(src).not.toContain('(${apiWhy})'); - }); - 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 cfErrorSummary. + // both must go through cfFailureTail, 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' @@ -835,16 +963,29 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { for (const source of [src, connectSrc]) { expect(source).not.toMatch(/JSON\.stringify\(res\.errors/); } - expect(src).toMatch(/cfErrorSummary\(res\.errors\)/); - // The empty-why case must not leave a trailing space after the status — - // pin the exact spacing ternary (a plain `) ${why}` mutant survives tests). - expect(src).toContain("${why ? ` ${why}` : ''}"); - // connect-domains' attach failures report through cfFailureTail, which - // sanitizes through the same cfErrorSummary and punctuates the reason - // itself — so the status also decides whether a scope is named at all. + expect(src).toContain('cfFailureTail(res.status, res.errors, PAGES_SCOPE_HINT)'); + // A bare status is what cfFailureTail replaced — "(HTTP 0)" for a thrown + // fetch was the meaningless line this pin now forbids coming back. + expect(src).not.toContain('(HTTP ${res.status})'); expect(connectSrc).toContain('cfFailureTail(res.status, res.errors, m.scopeHint)'); }); + 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('uploadThingTokenSet'); + 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)'); + expect(src).toMatch(/catch \{\s*seedOk = false;/); + }); + 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 (·) scope name reintroduced in an operator-facing diff --git a/scripts/setup-lib.ts b/scripts/setup-lib.ts index e8b07441..d7e8c5d1 100644 --- a/scripts/setup-lib.ts +++ b/scripts/setup-lib.ts @@ -394,6 +394,26 @@ export function statusLabel(status: number): string { return status === 0 ? '' : ` (HTTP ${status})`; } +/** 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}${statusLabel(status)}${cfFailureTail(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; @@ -547,6 +567,88 @@ export function securitySummaryLines(input: SecuritySummaryInput): string[] { 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 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 said R2 isn't enabled, + * 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). + */ +export function storageSummaryLines(input: StorageSummaryInput): string[] { + const { provider, r2Missing, uploadThingTokenSet, bucket, project } = input; + if (provider === 'r2') { + if (!r2Missing) return ['Storage backend: Cloudflare R2 (set up).']; + 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}` + ]; + } + 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}` + ]; +} + +/** + * 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 a549dd0d..dac1ec2d 100644 --- a/scripts/setup.ts +++ b/scripts/setup.ts @@ -41,8 +41,14 @@ import { imageResizingIsOn, ciWiringEntries, cfApi, - cfErrorSummary, + cfFailureTail, + statusLabel, + zoneLookupWarnLines, securitySummaryLines, + storageSummaryLines, + telegramSummaryLine, + setupTokenLines, + provisioningNoteLine, pagesPatchConfirmsSitekey, cdnAttachmentLines } from './setup-lib.ts'; @@ -126,12 +132,16 @@ 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' + + ` • ${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` + @@ -364,22 +374,15 @@ async function main() { resolvedZoneName = preflightZoneName; const zoneId = preflightZone.id; if (zoneLookupError !== null) { - // status 0 = fetch threw (no network). A 2xx means the API answered - // but its body said success:false, so repeat the API's own reason. - const apiWhy = - zoneLookupError >= 200 && zoneLookupError < 300 - ? cfErrorSummary(zoneLookupErrors) - : ''; - const why = - zoneLookupError === 0 - ? 'could not reach the Cloudflare API' - : `Cloudflare API error, HTTP ${zoneLookupError}${apiWhy ? `; the API said ${apiWhy}` : ''}`; - 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.` @@ -469,6 +472,18 @@ 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 @@ -522,15 +537,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 R2 isn't enabled. console.log( - '✔ attached D1/R2 bindings + FURTRACK_MODE to the Pages project (CI deploys get working bindings).' + `✔ attached D1${r2Missing ? '' : '/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'); - // Only the allowlisted code+message pairs — never the raw errors body, - // which can echo account/project identifiers into pasteable output. - const why = cfErrorSummary(res.errors); - console.warn(` (HTTP ${res.status})${why ? ` ${why}` : ''}`); + // cfFailureTail 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${statusLabel(res.status)}${cfFailureTail(res.status, res.errors, 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.'); @@ -583,10 +601,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(); @@ -605,10 +629,13 @@ 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); + // 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; if (resendApiKey) putSecret('RESEND_API_KEY', resendApiKey); if (resendFrom) putSecret('RESEND_FROM', resendFrom); // Turnstile secret for the admin-login siteverify. Server-only, so @@ -679,16 +706,13 @@ 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, 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)); 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`)'); @@ -734,15 +758,16 @@ async function main() { 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'); } From eb859e35179b8a0c28dbe085219d69bfeaa24c6f Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Thu, 20 Aug 2026 14:02:56 -0700 Subject: [PATCH 11/16] fix(setup): the twins of the last two fixes cdnDomainState got the malformed-body rule its pagesDomainState twin already had: findBucketDomain coerces either accepted shape to an empty list, so an ok response carrying neither read as 'absent' and green-lit the attach the read exists to gate. The Resend puts were the last secrets whose result was discarded. A supplied key whose put failed now says so by name, with the command that sets it, instead of surfacing months later as a dead reset link. --- scripts/connect-domains-lib.test.ts | 15 +++++++++++++++ scripts/connect-domains-lib.ts | 6 ++++++ scripts/setup-lib.test.ts | 29 +++++++++++++++++++++++++++++ scripts/setup-lib.ts | 16 ++++++++++++++++ scripts/setup.ts | 10 ++++++++-- 5 files changed, 74 insertions(+), 2 deletions(-) diff --git a/scripts/connect-domains-lib.test.ts b/scripts/connect-domains-lib.test.ts index 621a62e3..02365d72 100644 --- a/scripts/connect-domains-lib.test.ts +++ b/scripts/connect-domains-lib.test.ts @@ -207,6 +207,21 @@ describe('resolveZone', () => { 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' diff --git a/scripts/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index a9071a08..9e4e83bc 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -204,6 +204,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'; diff --git a/scripts/setup-lib.test.ts b/scripts/setup-lib.test.ts index 273a2fa1..d98075f5 100644 --- a/scripts/setup-lib.test.ts +++ b/scripts/setup-lib.test.ts @@ -26,6 +26,7 @@ import { zoneLookupWarnLines, storageSummaryLines, telegramSummaryLine, + resendSecretWarnLines, setupTokenLines, provisioningNoteLine, pagesPatchConfirmsSitekey, @@ -861,6 +862,34 @@ describe('storageSummaryLines', () => { }); }); +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'); diff --git a/scripts/setup-lib.ts b/scripts/setup-lib.ts index d7e8c5d1..a2b9971c 100644 --- a/scripts/setup-lib.ts +++ b/scripts/setup-lib.ts @@ -607,6 +607,22 @@ export function storageSummaryLines(input: StorageSummaryInput): string[] { * Telegram import hidden, and saying "enabled" would send the operator hunting in * the app for a feature that never turned on. */ +/** + * 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}`) + ]; +} + export function telegramSummaryLine(tokenProvided: boolean, tokenSet: boolean): string { if (!tokenProvided) return 'Telegram sticker import: not configured.'; return tokenSet diff --git a/scripts/setup.ts b/scripts/setup.ts index dac1ec2d..7564457f 100644 --- a/scripts/setup.ts +++ b/scripts/setup.ts @@ -47,6 +47,7 @@ import { securitySummaryLines, storageSummaryLines, telegramSummaryLine, + resendSecretWarnLines, setupTokenLines, provisioningNoteLine, pagesPatchConfirmsSitekey, @@ -636,8 +637,12 @@ async function main() { const uploadThingTokenSet = !useR2 && uploadThingToken ? putSecret('UPLOADTHING_TOKEN', uploadThingToken) : false; const telegramTokenSet = telegramBotToken ? putSecret('TELEGRAM_BOT_TOKEN', telegramBotToken) : false; - if (resendApiKey) putSecret('RESEND_API_KEY', resendApiKey); - if (resendFrom) putSecret('RESEND_FROM', resendFrom); + // 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. @@ -713,6 +718,7 @@ async function main() { `Fursuit photos: ${furtrackMode === 'off' ? 'disabled' : `enabled (${furtrackMode})`}${primaryCharacter ? ` — character "${primaryCharacter}"` : ''}.` ); 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`)'); From 82e575fa6923b030974ae5c9515b092b4549eee9 Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:37:34 -0700 Subject: [PATCH 12/16] fix(setup): decide the recipe from what happened, not from the wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isPermissionError searched the formatted detail for the scope hint, and the API's own message is echoed into that same text, so a 500 whose body quoted the permission read as a refusal and sent the operator to re-mint a token that was fine. Each call now records whether it was refused, and the runner reads that fact. Its recipe also lists Zone → Zone: Read, which the zone resolve needs and only setup's copy had. --- scripts/apply-download-ratelimit.ts | 9 ++++---- scripts/waf-lib.test.ts | 29 ++++++++++++++++++++--- scripts/waf-lib.ts | 36 ++++++++++++++++++++--------- 3 files changed, 56 insertions(+), 18 deletions(-) diff --git a/scripts/apply-download-ratelimit.ts b/scripts/apply-download-ratelimit.ts index cf5b869f..78b8aa28 100644 --- a/scripts/apply-download-ratelimit.ts +++ b/scripts/apply-download-ratelimit.ts @@ -24,6 +24,7 @@ import { applyDownloadRateLimit, isPermissionError } from './waf-lib.ts'; 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 → 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 -- '; @@ -58,10 +59,10 @@ async function main(): Promise { return 0; default: console.error(`✖ ${res.detail}\n`); - // The recipe fixes token scopes, so print it only when the detail names - // a permission failure (isPermissionError keys off waf-lib's own scope - // hint) — a transient HTTP error just gets plain retry guidance. - if (isPermissionError(res.detail)) { + // The recipe fixes token scopes, so print it only when the call actually + // hit a refusal (the result records that; the wording is not consulted) + // — a transient HTTP error just gets plain retry guidance. + if (isPermissionError(res)) { console.error(TOKEN_RECIPE); } else { console.error('This may not be a token-permission problem. Re-run once the'); diff --git a/scripts/waf-lib.test.ts b/scripts/waf-lib.test.ts index 744bffc4..251ee595 100644 --- a/scripts/waf-lib.test.ts +++ b/scripts/waf-lib.test.ts @@ -4,6 +4,7 @@ import { applyDownloadRateLimit, buildRule, isPermissionError, + SCOPE_HINT as WAF_SCOPE_HINT, RULE_REF, RULE_DESCRIPTION, RULE_EXPRESSION, @@ -615,14 +616,14 @@ describe('isPermissionError — the standalone runner’s recipe gate', () => { 'akito.dog', fakeApi({ [zonePath]: zoneOk, [entryPath]: { ok: false, status: 403 } }).api ); - expect(isPermissionError(denied.detail)).toBe(true); + expect(isPermissionError(denied)).toBe(true); const noAccess = await applyDownloadRateLimit( SECRET, 'akito.dog', fakeApi({ [zonePath]: { ok: true, status: 200, result: [] } }).api ); - expect(isPermissionError(noAccess.detail)).toBe(true); + expect(isPermissionError(noAccess)).toBe(true); const transient = await applyDownloadRateLimit( SECRET, @@ -633,6 +634,28 @@ describe('isPermissionError — the standalone runner’s recipe gate', () => { [putEntryPath]: { ok: false, status: 500 } }).api ); - expect(isPermissionError(transient.detail)).toBe(false); + 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 3fa963bd..fd20069f 100644 --- a/scripts/waf-lib.ts +++ b/scripts/waf-lib.ts @@ -130,8 +130,19 @@ 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; } +/** 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)'; @@ -142,14 +153,13 @@ function failureTail(status: number, errors?: unknown): string { } /** - * True when an error `detail` names a token-permission failure. Every branch - * that blames permissions interpolates SCOPE_HINT verbatim (via failureTail's - * 401/403 arm or the no-zone-access detail) and no other branch mentions it, - * so the standalone runner can decide whether the token recipe is relevant - * without RateLimitResult carrying an HTTP status. + * 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(detail: string): boolean { - return detail.includes(SCOPE_HINT); +export function isPermissionError(res: Pick): boolean { + return res.permissionDenied === true; } /** @@ -195,7 +205,8 @@ export async function applyDownloadRateLimit( if (errorStatus !== null) { return { status: 'error', - detail: `could not query zones for ${failedName ?? host}${statusLabel(errorStatus)}${failureTail(errorStatus, errors)}` + detail: `could not query zones for ${failedName ?? host}${statusLabel(errorStatus)}${failureTail(errorStatus, errors)}`, + permissionDenied: deniedByScope(errorStatus) }; } zoneId = zone.id; @@ -203,7 +214,8 @@ export async function applyDownloadRateLimit( if (!zoneId) { return { status: 'error', - detail: `the token has no access to the ${host} zone; add ${SCOPE_HINT}` + detail: `the token has no access to the ${host} zone; add ${SCOPE_HINT}`, + permissionDenied: true }; } @@ -219,7 +231,8 @@ export async function applyDownloadRateLimit( // 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}${statusLabel(entry.status)}${failureTail(entry.status, entry.errors)}` + detail: `could not read the rate-limit ruleset for ${host}${statusLabel(entry.status)}${failureTail(entry.status, entry.errors)}`, + permissionDenied: deniedByScope(entry.status) }; } @@ -259,7 +272,8 @@ export async function applyDownloadRateLimit( // on this write. Any other status gets no scope advice. return { status: 'error', - detail: `failed to write the rate-limit rule to ${host}${statusLabel(write.status)}${failureTail(write.status, write.errors)}` + detail: `failed to write the rate-limit rule to ${host}${statusLabel(write.status)}${failureTail(write.status, write.errors)}`, + permissionDenied: deniedByScope(write.status) }; } return mine From 6be80baf007eead6370ec80a532a99a4fd81857e Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:44:26 -0700 Subject: [PATCH 13/16] fix(connect-domains): guard malformed domain entries; tidy two docs A non-object entry in the bucket domain list threw on the property read instead of reporting the unknown state its caller exists to return. The Telegram docblock went back above its own function, and UPDATING.md now names both permissions the rate-limit command needs. --- UPDATING.md | 5 +++-- scripts/connect-domains-lib.ts | 4 +++- scripts/setup-lib.ts | 12 ++++++------ 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/UPDATING.md b/UPDATING.md index 056e7fe9..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/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index 9e4e83bc..b83feffd 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -187,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) => typeof d === 'object' && d !== null && d.domain === name); } /** diff --git a/scripts/setup-lib.ts b/scripts/setup-lib.ts index a2b9971c..9000bab1 100644 --- a/scripts/setup-lib.ts +++ b/scripts/setup-lib.ts @@ -601,12 +601,6 @@ export function storageSummaryLines(input: StorageSummaryInput): string[] { ]; } -/** - * 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. - */ /** * 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 @@ -623,6 +617,12 @@ export function resendSecretWarnLines(failed: string[], project: string): string ]; } +/** + * 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 From 1deae6db4e1f1d33fee12cdbdcbd2dcf33422840 Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:56:32 -0700 Subject: [PATCH 14/16] fix(setup): guard the two remaining unvalidated list reads Found by sweeping for the pattern rather than waiting for it to be reported: pagesDomainAttached and the WAF rule reconcile both read a property off list entries without checking they are objects, the same defect already fixed in findBucketDomain. A junk entry threw where the caller expected a non-match. --- scripts/connect-domains-lib.test.ts | 12 ++++++++++++ scripts/connect-domains-lib.ts | 4 +++- scripts/waf-lib.ts | 5 ++++- 3 files changed, 19 insertions(+), 2 deletions(-) diff --git a/scripts/connect-domains-lib.test.ts b/scripts/connect-domains-lib.test.ts index 02365d72..c3d9af71 100644 --- a/scripts/connect-domains-lib.test.ts +++ b/scripts/connect-domains-lib.test.ts @@ -204,6 +204,18 @@ 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 } }); diff --git a/scripts/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index b83feffd..f84dd222 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -228,7 +228,9 @@ 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) => typeof d === 'object' && d !== null && d.name === host); } /** diff --git a/scripts/waf-lib.ts b/scripts/waf-lib.ts index fd20069f..13d5e8b0 100644 --- a/scripts/waf-lib.ts +++ b/scripts/waf-lib.ts @@ -239,7 +239,10 @@ export async function applyDownloadRateLimit( // 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) => typeof r === 'object' && r !== null && r.ref === RULE_REF); if (mine && ruleMatches(mine)) { return { status: 'exists', detail: `rate-limit rule already present on ${host} — no change` }; } From b99650df3268ede3806f1e6409df784959055e34 Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:19:55 -0700 Subject: [PATCH 15/16] fix(setup): the defects a review round found, and the tests that missed them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness: - The zone lookup named Zone → WAF: Edit when it needs Zone → Zone: Read, and an empty zone list asserted a permission refusal, so a domain that simply isn't on the account printed the whole token recipe. It now names the right scope and leaves permissionDenied unset for a missing zone. - An ok ruleset body whose rules field wasn't an array threw out of applyDownloadRateLimit, aborting setup after D1, R2 and Pages had been written. It returns a no-mutation error instead. - cfApi treats ok as success, so the malformed-body paths were printing 'the API reported failure' for responses that reported success. They now say the response carried no domain list. - The R2 backend was called set up on the strength of a stderr text sniff, so a token without Workers R2 Storage: Edit bound a bucket that did not exist. The create's outcome is recorded and both claims gate on it, with an already-exists re-run still counting as success. Tests that did not test: - The runner's recipe gate had no test at all; inverting it passed. - permissionDenied on the zone-lookup and rule-write refusals, the WAF reconcile entry guard, and readFailureTail's never-ran branch were all mutable without failing anything. - A source pin read as a substring, so uploadThingTokenSet: true satisfied the check meant to prove a variable was passed. Also: format characters are now removed rather than spaced, so a zero width inside an id can no longer split it past the scrub; ids adjacent to an underscore scrub too; API-derived path segments are encoded; and the notation guard lost its hand-rolled comment and fence parsers now that the six stale comments are converted. --- scripts/apply-download-ratelimit.test.ts | 47 ++++++ scripts/apply-download-ratelimit.ts | 57 +++++--- scripts/connect-domains-lib.test.ts | 32 +++- scripts/connect-domains-lib.ts | 36 +++-- scripts/connect-domains.test.ts | 69 ++++----- scripts/connect-domains.ts | 41 ++---- scripts/setup-lib.test.ts | 177 ++++++++++++++++------- scripts/setup-lib.ts | 76 ++++++++-- scripts/setup.ts | 54 +++++-- scripts/turnstile-lib.test.ts | 23 +++ scripts/turnstile-lib.ts | 18 ++- scripts/waf-lib.test.ts | 78 ++++++++-- scripts/waf-lib.ts | 62 +++++--- 13 files changed, 557 insertions(+), 213 deletions(-) create mode 100644 scripts/apply-download-ratelimit.test.ts 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 78b8aa28..96282e1d 100644 --- a/scripts/apply-download-ratelimit.ts +++ b/scripts/apply-download-ratelimit.ts @@ -15,13 +15,14 @@ * 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, isPermissionError } 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 → Zone: Read\n' + @@ -29,6 +30,27 @@ const TOKEN_RECIPE = ' 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'); @@ -58,25 +80,18 @@ async function main(): Promise { console.log(`✔ ${res.detail}`); return 0; default: - console.error(`✖ ${res.detail}\n`); - // The recipe fixes token scopes, so print it only when the call actually - // hit a refusal (the result records that; the wording is not consulted) - // — a transient HTTP error just gets plain retry guidance. - if (isPermissionError(res)) { - console.error(TOKEN_RECIPE); - } else { - console.error('This may not be a token-permission problem. Re-run once the'); - console.error('reason above is resolved:'); - console.error(' CLOUDFLARE_API_TOKEN= npm run apply-download-ratelimit -- '); - } + 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 c3d9af71..65b0c370 100644 --- a/scripts/connect-domains-lib.test.ts +++ b/scripts/connect-domains-lib.test.ts @@ -544,14 +544,32 @@ describe('buildLadder + firstFailingRung', () => { 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', () => { - const cdnAction = (over: Partial) => - buildLadder({ ...healthy, cdnState: 'unknown', tlsIssued: null, ...over }).find( - (r) => r.id === 'cdn-attached' - )!.action!; - expect(cdnAction({ cdnReadStatus: 403 })).toContain('token needs Account → Workers R2 Storage: Read'); - expect(cdnAction({ cdnReadStatus: 503 })).not.toContain('Account → Workers R2 Storage: Read'); - expect(cdnAction({ cdnReadStatus: 0 })).toContain('the Cloudflare API did not respond'); + 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'); }); }); diff --git a/scripts/connect-domains-lib.ts b/scripts/connect-domains-lib.ts index f84dd222..bb7b9ae4 100644 --- a/scripts/connect-domains-lib.ts +++ b/scripts/connect-domains-lib.ts @@ -11,7 +11,7 @@ * 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 { cfFailureTail, statusLabel, type CfApiResult } from './setup-lib.ts'; +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 { @@ -189,7 +189,7 @@ export function findBucketDomain(result: unknown, name: string): BucketDomain | (Array.isArray(result) ? (result as BucketDomain[]) : [])) as BucketDomain[]; // 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) => typeof d === 'object' && d !== null && d.domain === name); + return list.find((d) => isRecord(d) && d.domain === name); } /** @@ -197,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'; @@ -230,7 +230,7 @@ export function pagesDomainAttached(result: unknown, host: string): boolean { const list = (Array.isArray(result) ? result : []) as { name?: string }[]; // Same guard as findBucketDomain: a non-object entry would throw on the // property read rather than simply failing to match. - return list.some((d) => typeof d === 'object' && d !== null && d.name === host); + return list.some((d) => isRecord(d) && d.name === host); } /** @@ -359,9 +359,8 @@ export interface LadderInputs { zoneActive: boolean; /** attached = healthy, absent = not there, disabled = present-but-off, unknown = R2 read failed. */ cdnState: CdnDomainState; - /** The R2 read's HTTP status + errors body, so an `unknown` rung can say WHY. */ - cdnReadStatus?: number; - cdnReadErrors?: unknown; + /** 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 (the settings read failed). */ @@ -383,7 +382,23 @@ export interface LadderInputs { * would rather say nothing than name a cause we never observed. */ function readFailureTail(status: number | undefined, errors: unknown, scopeHint: string): string { - return status === undefined ? '' : `${statusLabel(status)}${cfFailureTail(status, errors, scopeHint)}`; + 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); } /** @@ -431,9 +446,8 @@ export function buildLadder(i: LadderInputs): Rung[] { i.cdnState === 'attached' ? 'pass' : i.cdnState === 'absent' ? 'fail' : 'warn'; const cdnAction = i.cdnState === 'unknown' - ? `Couldn't verify${readFailureTail( - i.cdnReadStatus, - i.cdnReadErrors, + ? `Couldn't verify${domainReadFailure( + i.cdnRead, 'Account → Workers R2 Storage: Read' )}. Check the bucket's Custom Domains in the dashboard.` : i.cdnState === 'disabled' diff --git a/scripts/connect-domains.test.ts b/scripts/connect-domains.test.ts index 750f9cbf..0956444a 100644 --- a/scripts/connect-domains.test.ts +++ b/scripts/connect-domains.test.ts @@ -129,6 +129,28 @@ 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". @@ -222,13 +244,12 @@ describe('connect-domains.ts ↔ candidate-walk source contract', () => { // 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);'); - expect(src).not.toContain('errorStatus >= 200 && errorStatus < 300'); }); }); -// Every other failed call in the mutating path reports through cfFailureTail, 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 +// 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', () => { @@ -237,39 +258,24 @@ describe('connect-domains.ts ↔ failure-reporting source contract', () => { 'utf8' ); - it('reports the R2 custom-domain read through cfFailureTail', () => { - expect(src).toMatch(/cfFailureTail\(\s*r2Res\.status,\s*r2Res\.errors,\s*'Account → Workers R2 Storage: Read'/); - expect(src).not.toContain('token may lack Account → Workers R2 Storage: Read'); + // 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, never a bare HTTP 0", () => { - expect(src).toContain('cfFailureTail(res.status, res.errors, m.scopeHint)'); - expect(src).not.toContain('(HTTP ${res.status})'); + 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(/cfFailureTail\(\s*patched\.status,\s*patched\.errors,\s*'Zone → Zone Settings: Edit'/); - expect(src).not.toContain('(HTTP ${patched.status})'); - }); - - it('uses statusLabel so a thrown fetch prints no status at all', () => { - expect(src).toContain('statusLabel(r2Res.status)'); - expect(src).toContain('statusLabel(res.status)'); - expect(src).toContain('statusLabel(patched.status)'); - }); - - it('reports the Pages-domain read through cfFailureTail as well', () => { - expect(src).toMatch( - /cfFailureTail\(\s*pagesRes\.status,\s*pagesRes\.errors,\s*'Account → Cloudflare Pages: Read'/ - ); - expect(src).toContain('statusLabel(pagesRes.status)'); + 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( - /cfFailureTail\(\s*irGet\.status,\s*irGet\.errors,\s*'Zone → Zone Settings: Read'/ - ); - expect(src).not.toContain('token lacks Zone → Zone Settings: Read'); + expect(src).toMatch(/failureDetail\(\s*irGet,\s*'Zone → Zone Settings: Read'/); }); }); @@ -286,9 +292,6 @@ describe('connect-domains.ts ↔ never-mutate-on-an-unread-state contract', () = 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'"); - // The old wiring read the list off a possibly-failed response, so a 403 - // became "not attached" and the attach went out anyway. - expect(src).not.toContain('pagesDomainAttached(pagesRes.result, host)'); }); it('skips the attach and says so when either read came back unknown', () => { @@ -298,6 +301,6 @@ describe('connect-domains.ts ↔ never-mutate-on-an-unread-state contract', () = }); it('never claims "Already connected" off a read that failed', () => { - expect(src).toMatch(/const unread = cdnState === 'unknown' \? cdn : pagesState === 'unknown' \? host : null/); + 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 c4439f28..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. * @@ -33,8 +33,7 @@ import { fileURLToPath } from 'node:url'; import { cfApi, cfErrorSummary, - cfFailureTail, - statusLabel, + failureDetail, hostFromDomain, zoneNameCandidates, imageResizingOutcome, @@ -51,6 +50,7 @@ import { bucketDomainTlsIssued, pagesDomainState, classifyCdnProbe, + domainReadFailure, planConnect, siteUrlMismatch, buildLadder, @@ -205,9 +205,8 @@ async function main(): Promise { // 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${statusLabel(r2Res.status)}${cfFailureTail( - r2Res.status, - r2Res.errors, + `⚠ Couldn't read the bucket's custom domains${domainReadFailure( + r2Res, 'Account → Workers R2 Storage: Read' )} — skipping the ${cdn} attach.` ); @@ -217,9 +216,8 @@ async function main(): Promise { // 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${statusLabel(pagesRes.status)}${cfFailureTail( - pagesRes.status, - pagesRes.errors, + `⚠ Couldn't read the ${project} Pages project's domains${domainReadFailure( + pagesRes, 'Account → Cloudflare Pages: Read' )} — skipping the ${host} attach.` ); @@ -253,11 +251,7 @@ async function main(): Promise { console.log( transformsCurrent === true ? ' Image Transformations: on.' - : ` Image Transformations: couldn't verify${statusLabel(irGet.status)}${cfFailureTail( - irGet.status, - irGet.errors, - 'Zone → 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; @@ -299,12 +293,12 @@ async function main(): Promise { const res = await cfApi(cfToken, m.path, { method: m.method, body: m.body }); if (res.ok) console.log(`✔ ${m.label}`); else - // cfFailureTail keeps the reason honest per status: the call's own scope + // 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}${statusLabel(res.status)}${cfFailureTail(res.status, res.errors, m.scopeHint)}` + `⚠ Could not ${m.label}${failureDetail(res, m.scopeHint)}` ); } @@ -316,21 +310,13 @@ async function main(): Promise { if (patched.ok) console.log(`✔ Image Transformations enabled on ${zoneLabel}.`); else console.warn( - `⚠ Could not enable Image Transformations${statusLabel(patched.status)}${cfFailureTail( - patched.status, - patched.errors, - 'Zone → Zone Settings: Edit' - )} — 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${statusLabel(irGet.status)}${cfFailureTail( - irGet.status, - irGet.errors, - 'Zone → Zone Settings: Read' - )} — enable it in the dashboard.` + `⚠ Image Transformations: couldn't verify${failureDetail(irGet, 'Zone → Zone Settings: Read')} — enable it in the dashboard.` ); } @@ -417,8 +403,7 @@ export async function runDoctor(a: DoctorArgs, deps: DoctorDeps = defaultDoctorD zoneName: a.zoneName, candidates: a.candidates, cdnState, - cdnReadStatus: r2Read?.status, - cdnReadErrors: r2Read?.errors, + cdnRead: r2Read, tlsIssued, imageTransforms: transforms, imageTransformsStatus: irRead?.status, diff --git a/scripts/setup-lib.test.ts b/scripts/setup-lib.test.ts index d98075f5..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, @@ -180,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'); @@ -670,7 +693,7 @@ describe('securitySummaryLines', () => { const detail = await realRateLimitDetail({ 'GET /zones?name=taro.surf': { ok: true, status: 200, result: [] } }); - expect(detail).toContain('no access to the taro.surf zone'); + 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}.`); @@ -835,26 +858,56 @@ describe('storageSummaryLines', () => { it('reports R2 as set up, and NOT READY when R2 is not enabled', () => { expect( - storageSummaryLines({ ...base, provider: 'r2', r2Missing: false, uploadThingTokenSet: false }) + 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, uploadThingTokenSet: true }) + 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)'); @@ -983,7 +1036,7 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { 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 cfFailureTail, which reads only the allowlisted + // 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'), @@ -992,11 +1045,7 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { for (const source of [src, connectSrc]) { expect(source).not.toMatch(/JSON\.stringify\(res\.errors/); } - expect(src).toContain('cfFailureTail(res.status, res.errors, PAGES_SCOPE_HINT)'); - // A bare status is what cfFailureTail replaced — "(HTTP 0)" for a thrown - // fetch was the meaningless line this pin now forbids coming back. - expect(src).not.toContain('(HTTP ${res.status})'); - expect(connectSrc).toContain('cfFailureTail(res.status, res.errors, m.scopeHint)'); + expect(src).toContain('failureDetail(res, PAGES_SCOPE_HINT)'); }); it('stops rather than wire an empty D1 database_id', () => { @@ -1008,61 +1057,65 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { 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('uploadThingTokenSet'); 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)'); - expect(src).toMatch(/catch \{\s*seedOk = false;/); + }); + + // 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 (·) scope name reintroduced in an operator-facing - // surface. Code comments are out of scope; connect-domains-lib's '·' - // skip-glyph is the one allowed code occurrence. + // 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('·'); - // The recipes' positive pins, and apply-download-ratelimit's negative one - // (its file is not in the whole-file scan below, which subsumes the rest). - const recipeOf = (file: string) => { - const source = readFileSync(join(here, file), 'utf8'); - const recipe = source.match(/const TOKEN_RECIPE =[\s\S]*?';/)?.[0] ?? ''; - expect(recipe, `${file} TOKEN_RECIPE found`).not.toBe(''); - return recipe; - }; - expect(recipeOf('apply-download-ratelimit.ts')).not.toContain('·'); - expect(recipeOf('setup.ts')).toContain('Zone → Zone: Read'); - expect(recipeOf('setup.ts')).toContain('needed later to attach the apex record'); - - // Whole-file scan of the CLIs' non-comment code (printed strings live - // there): drop full-line and trailing // comments (a URL's :// never - // matches — the strip needs whitespace or line start before //), minus - // the one legal skip-glyph. - for (const file of ['setup.ts', 'connect-domains.ts', 'connect-domains-lib.ts']) { - const code = readFileSync(join(here, file), 'utf8') - .split('\n') - .filter((l) => !/^\s*(\/\/|\*|\/\*)/.test(l)) - .map((l) => l.replace(/(^|\s)\/\/.*$/, '')) - .filter((l) => !l.includes("skip: '·'")) - .join('\n'); - expect(code, `${file} non-comment code`).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('·'); } - // The two connect-domains transforms strings now hand the scope to - // cfFailureTail, which prints it only on a 401/403, so pin the arrow form at - // those call sites (positive pins so a rewrite can't drop the scope name) - // and pin out the old sentence that blamed the scope for every failure. - const connectSource = readFileSync(join(here, 'connect-domains.ts'), 'utf8'); - expect( - connectSource.match( - /cfFailureTail\(\s*irGet\.status,\s*irGet\.errors,\s*'Zone → Zone Settings: Read'/g - ) - ).toHaveLength(2); - expect(connectSource).not.toContain('token lacks Zone → Zone Settings: Read'); + // 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']) { @@ -1090,13 +1143,8 @@ describe('setup.ts ↔ securitySummaryLines call-site contract', () => { }); 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', () => { @@ -1191,6 +1239,25 @@ describe('cfErrorSummary', () => { 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' } diff --git a/scripts/setup-lib.ts b/scripts/setup-lib.ts index 9000bab1..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,15 @@ 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 => { @@ -337,19 +359,26 @@ const capPoints = (value: string, max: number): string => { export function cfErrorSummary(errors: unknown): string { if (!Array.isArray(errors)) return ''; const joined = errors - .filter((e): e is { code?: unknown; message?: unknown } => typeof e === 'object' && e !== null) + .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{Cc}\p{Cf}]/gu, ' ') + .replace(/\p{Cf}/gu, '') + .replace(/\p{Cc}/gu, ' ') .replace(/\s+/g, ' ') .trim() - .replace(/\b[0-9a-f]{32}\b/gi, '') + .replace(/(?') : ''; if (!raw) return ''; const message = capPoints(raw, 200); @@ -394,6 +423,15 @@ 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'; @@ -409,7 +447,7 @@ export const ZONE_READ_SCOPE_HINT = 'Zone → Zone: Read'; */ export function zoneLookupWarnLines(name: string, status: number, errors: unknown): string[] { return [ - `\n⚠ Zone lookup failed for ${name}${statusLabel(status)}${cfFailureTail(status, errors, ZONE_READ_SCOPE_HINT)} — skipping the DNS / image-transform preflight.`, + `\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.' ]; } @@ -572,6 +610,8 @@ export interface StorageSummaryInput { 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; @@ -580,18 +620,28 @@ export interface StorageSummaryInput { /** * The end-of-run "Storage backend:" lines. Both backends report NOT READY on the - * state we actually established: R2 when the bucket create said R2 isn't enabled, - * 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). + * 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, uploadThingTokenSet, bucket, project } = input; + const { provider, r2Missing, bucketReady, uploadThingTokenSet, bucket, project } = input; if (provider === 'r2') { - if (!r2Missing) return ['Storage backend: Cloudflare R2 (set up).']; + 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 (R2 is not enabled on this account).', - ` Create the bucket, then re-run setup: npx wrangler r2 bucket create ${bucket}` + `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).']; diff --git a/scripts/setup.ts b/scripts/setup.ts index 7564457f..ba7b7f06 100644 --- a/scripts/setup.ts +++ b/scripts/setup.ts @@ -27,6 +27,7 @@ import { buildMigrationSql, buildSeedSql, sanitizeProjectName, + bucketCreateSucceeded, isR2NotEnabled, ensureUrlScheme, ghSecretEligibility, @@ -41,8 +42,7 @@ import { imageResizingIsOn, ciWiringEntries, cfApi, - cfFailureTail, - statusLabel, + failureDetail, zoneLookupWarnLines, securitySummaryLines, storageSummaryLines, @@ -80,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}`); @@ -93,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 ?? ''}`; } @@ -319,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 @@ -413,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; @@ -489,15 +494,31 @@ async function main() { // 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. @@ -520,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. @@ -538,17 +561,17 @@ 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 R2 isn't enabled. + // Name only what was sent: the R2 binding is omitted when no bucket exists. console.log( - `✔ attached D1${r2Missing ? '' : '/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 { - // cfFailureTail keeps the reason honest per status — a thrown fetch says + // 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${statusLabel(res.status)}${cfFailureTail(res.status, res.errors, PAGES_SCOPE_HINT)}` + `\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'); @@ -711,7 +734,14 @@ async function main() { rl.close(); console.log('\n──────────────────────────────────────────────'); - for (const line of storageSummaryLines({ provider, r2Missing, uploadThingTokenSet, bucket, project })) { + for (const line of storageSummaryLines({ + provider, + r2Missing, + bucketReady, + uploadThingTokenSet, + bucket, + project + })) { console.log(line); } console.log( diff --git a/scripts/turnstile-lib.test.ts b/scripts/turnstile-lib.test.ts index 9832ad29..2483e62f 100644 --- a/scripts/turnstile-lib.test.ts +++ b/scripts/turnstile-lib.test.ts @@ -120,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 diff --git a/scripts/turnstile-lib.ts b/scripts/turnstile-lib.ts index 93f30556..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, cfFailureTail, hostFromDomain, statusLabel, type CfApiResult } 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) @@ -140,7 +147,7 @@ export async function provisionTurnstileWidget( if (!listRes.ok) { return { status: 'error', - detail: `could not list Turnstile widgets${statusLabel(listRes.status)}${failureTail(listRes)}` + 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. @@ -188,7 +195,12 @@ export async function provisionTurnstileWidget( // 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 diff --git a/scripts/waf-lib.test.ts b/scripts/waf-lib.test.ts index 251ee595..221dd53d 100644 --- a/scripts/waf-lib.test.ts +++ b/scripts/waf-lib.test.ts @@ -240,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 the akito.dog zone'); - 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); }); @@ -287,9 +290,11 @@ describe('applyDownloadRateLimit — clear errors, no mutation', () => { // 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. - expect(res.detail).toContain('token needs'); - expect(res.detail).toContain('WAF: Edit'); + // 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); }); @@ -370,6 +375,59 @@ describe('applyDownloadRateLimit — clear errors, no mutation', () => { 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); @@ -618,12 +676,14 @@ describe('isPermissionError — the standalone runner’s recipe gate', () => { ); expect(isPermissionError(denied)).toBe(true); - const noAccess = await applyDownloadRateLimit( + // 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: true, status: 200, result: [] } }).api + fakeApi({ [zonePath]: { ok: false, status: 403 } }).api ); - expect(isPermissionError(noAccess)).toBe(true); + expect(isPermissionError(zoneDenied)).toBe(true); const transient = await applyDownloadRateLimit( SECRET, diff --git a/scripts/waf-lib.ts b/scripts/waf-lib.ts index 13d5e8b0..e87d02c0 100644 --- a/scripts/waf-lib.ts +++ b/scripts/waf-lib.ts @@ -14,10 +14,12 @@ */ import { cfApi, - cfFailureTail, + failureDetail, hostFromDomain, + isRecord, statusLabel, zoneNameCandidates, + ZONE_READ_SCOPE_HINT, type CfApiResult } from './setup-lib.ts'; import { resolveZone } from './connect-domains-lib.ts'; @@ -147,11 +149,6 @@ const deniedByScope = (status: number): boolean => status === 401 || status === * 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)'; -/** setup-lib's shared cfFailureTail, bound to this lib's scope hint. */ -function failureTail(status: number, errors?: unknown): string { - return cfFailureTail(status, errors, SCOPE_HINT); -} - /** * 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 @@ -167,8 +164,9 @@ export function isPermissionError(res: Pick * * 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. @@ -203,19 +201,25 @@ export async function applyDownloadRateLimit( (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}${statusLabel(errorStatus)}${failureTail(errorStatus, errors)}`, + 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: `the token has no access to the ${host} zone; add ${SCOPE_HINT}`, - permissionDenied: true + detail: `no ${host} zone was found on this Cloudflare account — check the domain, the account, and that the token's Zone Resources include it` }; } @@ -224,14 +228,24 @@ export async function applyDownloadRateLimit( 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}${statusLabel(entry.status)}${failureTail(entry.status, entry.errors)}`, + detail: `could not read the rate-limit ruleset for ${host}${failureDetail(entry, SCOPE_HINT)}`, permissionDenied: deniedByScope(entry.status) }; } @@ -242,22 +256,28 @@ export async function applyDownloadRateLimit( // 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) => typeof r === 'object' && r !== null && r.ref === RULE_REF); + 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/${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/${zoneId}/rulesets/${encodeURIComponent(rulesetId)}/rules`, { method: 'POST', body: buildRule() }); @@ -275,7 +295,7 @@ export async function applyDownloadRateLimit( // on this write. Any other status gets no scope advice. return { status: 'error', - detail: `failed to write the rate-limit rule to ${host}${statusLabel(write.status)}${failureTail(write.status, write.errors)}`, + detail: `failed to write the rate-limit rule to ${host}${failureDetail(write, SCOPE_HINT)}`, permissionDenied: deniedByScope(write.status) }; } From afac65b36076f23c1728750baba87a2a876db8f5 Mon Sep 17 00:00:00 2001 From: Sparky <1609870+sparkyfen@users.noreply.github.com> Date: Sat, 22 Aug 2026 11:06:18 -0700 Subject: [PATCH 16/16] fix(setup): encode zoneId in the write paths too The comment beside these calls says API-returned ids are encoded like any other untrusted path segment, but only rulesetId and the rule id were. zoneId comes from the same place, so the comment now describes the code. --- scripts/waf-lib.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/waf-lib.ts b/scripts/waf-lib.ts index e87d02c0..71bc429e 100644 --- a/scripts/waf-lib.ts +++ b/scripts/waf-lib.ts @@ -224,7 +224,7 @@ export async function applyDownloadRateLimit( } // 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) { @@ -268,7 +268,7 @@ export async function applyDownloadRateLimit( // Param bump: update just our rule in place. return api( cfToken, - `/zones/${zoneId}/rulesets/${encodeURIComponent(rulesetId)}/rules/${encodeURIComponent(mine.id)}`, + `/zones/${encodeURIComponent(zoneId)}/rulesets/${encodeURIComponent(rulesetId)}/rules/${encodeURIComponent(mine.id)}`, { method: 'PATCH', body: buildRule() @@ -277,13 +277,13 @@ export async function applyDownloadRateLimit( } if (rulesetId) { // Ruleset exists, our rule is absent: append only our rule. - return api(cfToken, `/zones/${zoneId}/rulesets/${encodeURIComponent(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()] } });