From da2afe3df8966e28f7a3980f9ddebd4c7e1879d0 Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Sun, 24 May 2026 20:04:16 -0500 Subject: [PATCH] =?UTF-8?q?convoy:=20scope=20cors-tighten=20(P0=20#5=20PAR?= =?UTF-8?q?TIAL=20=E2=86=92=20RESOLVED)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scaffolds the cors-tighten convoy. Originally queued as a one-file follow-up to fix-auth-bypass Brief 4 (only pages/api/auth/verify.js), but a fresh repo-wide grep at convoy creation found 24 files carrying the identical scaffolded wildcard-CORS + redundant-OPTIONS pattern. The documented narrow scope (verify.js only) materially understates the actual exposure. Two scope options spec'd in the convoy file for architect ratification at gate 1: Option A — narrow (verify.js only, ~10 LOC). Matches the queued-convoy entry literally. Queues a separate cors-sweep-all-routes for the remaining 23. Option B — expanded (all 24 files, ~240 LOC). Single mechanical sweep, same precedent as Brief 4 applied to login + register, closes P0 #5 fully in one PR. Parent recommends Option B. Architect verifies scope safety (spot-check 5+ of 24 for pattern drift), confirms or splits, adds optional CI regression lock if B wins. Four decisions queued (scope option, OPTIONS-handler shape, verify.js method-gate tightening, test coverage). No operator action required — no env vars, no secrets, no infra. Co-authored-by: Cursor --- .convoys/cors-tighten.md | 242 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 .convoys/cors-tighten.md diff --git a/.convoys/cors-tighten.md b/.convoys/cors-tighten.md new file mode 100644 index 0000000..f5425c9 --- /dev/null +++ b/.convoys/cors-tighten.md @@ -0,0 +1,242 @@ +--- +name: cors-tighten +classification: convoy +success_metric: | + No `pages/api/**/*.js` handler ships an `Access-Control-Allow-Origin: *` + header (or any other wildcard CORS header), AND no handler ships a + same-origin redundant OPTIONS preflight handler. Browser-issued + cross-origin POSTs to the auth surface return a CORS error instead of + succeeding. `npm run test:smoke` continues to pass (the smoke spec is + same-origin via the Vercel preview URL, so it is unaffected). +skip: + - role-design-system-auditor + - role-a11y-auditor + - role-ux-reviewer + - role-ia-architect +status: queued +created: 2026-05-24 +parent: ship-readiness +addresses: P0 #5 (PARTIAL → RESOLVED) +depends_on: + - fix-auth-bypass (Brief 4, shipped — login + register CORS removal is the precedent) +--- + +# Convoy: cors-tighten + +Drop wildcard `Access-Control-Allow-Origin: *` from the remaining +`pages/api/**` handlers. The `fix-auth-bypass` Brief 4 already +cleaned login + register; the documented follow-up was just +`pages/api/auth/verify.js`, but a fresh audit (parent grep at +convoy creation, 2026-05-24) found **24 files** repo-wide carrying +the identical scaffolded pattern: + +```js +// Set CORS headers +res.setHeader('Access-Control-Allow-Origin', '*'); +res.setHeader('Access-Control-Allow-Methods', '...'); +res.setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization'); + +// Handle preflight requests +if (req.method === 'OPTIONS') { + res.status(200).end(); + return; +} +``` + +The 24 files (spot-checked against `pages/api/cards/search.js`, +`pages/api/collections.js`, `pages/api/user/avatar.js` — all +identical except for the `Allow-Methods` verb list): + +``` +pages/api/admin/index.js +pages/api/auth/verify.js +pages/api/cards/[id]/ownership.js +pages/api/cards/owned.js +pages/api/cards/search.js +pages/api/collections.js +pages/api/collections/[identifier].js +pages/api/collections/[identifier]/activity.js +pages/api/collections/[identifier]/cards.js +pages/api/collections/[identifier]/permissions.js +pages/api/collections/[identifier]/thumbnails.js +pages/api/community/collections.js +pages/api/favorites.js +pages/api/invite/accept.js +pages/api/invite/decline.js +pages/api/public/collections.js +pages/api/user/avatar.js +pages/api/user/avatar/generate.js +pages/api/user/delete.js +pages/api/user/password.js +pages/api/user/profile.js +pages/api/user/settings.js +pages/api/user/stats.js +pages/api/users/search.js +``` + +## Why now + +P0 #5 in `.convoys/ship-readiness.md` was marked PARTIAL on +2026-05-23 because Brief 4 only fixed the auth surface (login, +register) under that convoy's narrow auth-bypass mandate. The +"Queued convoys" entry assumed `cors-tighten` would be a one-file +follow-up on `verify.js`. The 23-file gap is a fresh discovery. + +The wildcard `Access-Control-Allow-Origin: *` allows any origin to +read API responses from authenticated browser sessions. Combined +with the JWT-in-`Authorization`-header pattern this is less +exploitable than cookie-based sessions would be (browsers won't +attach the token automatically across origins), but the wildcard +still: + +1. **Enables credential stuffing from third-party origins** — + attacker can serve a page that POSTs to `/api/auth/login` with + guessed credentials and read the response (success/failure + + token). The 5-attempt/15-min rate limit from + `lib/rate-limit.js` mitigates volume but not the + attack-class. +2. **Enables arbitrary cross-origin reads of any authenticated GET + response** if a victim manually attaches a Bearer token in the + wrong browser context (or if a downstream consumer ever pivots + to cookies, which `single-auth-provider` may eventually do). +3. **Defeats Vercel's same-origin-by-default deployment shape** — + `tcg-vault.com` (or whatever it ends up being post-`pick-a-name`) + and the API are served from the same Vercel project. There is + no legitimate cross-origin caller. The header is purely + scaffolding cruft from whatever generator created the original + route templates. + +This convoy is launch sequence step 4 in `.convoys/ship-readiness.md`'s +"Proposed launch sequence" (originally `add-rate-limiting`'s slot, +but `cors-tighten` was queued separately and is logically prior — +fixing CORS first means rate-limiting's protection isn't +side-stepped by a cross-origin caller). + +## Scope — TWO OPTIONS, architect picks at gate 1 + +### Option A — Narrow (matches the documented queued convoy entry) + +- Drop wildcard CORS + OPTIONS handler from `pages/api/auth/verify.js` + ONLY. ~10 LOC deletion. Closes P0 #5 from PARTIAL → RESOLVED-on-auth-surface. +- Queue a separate `cors-sweep-all-routes` convoy for the remaining + 23 files. Adds friction (two PRs, two doc-writer cleanups), but + matches the convoy's original documented scope. + +### Option B — Expanded (recommended by parent, pending architect ratification) + +- Drop wildcard CORS + OPTIONS handler from **all 24 files** in one + PR. ~240 LOC deletion across 24 files, mechanically identical to + what Brief 4 did to login + register. Closes P0 #5 fully — + PARTIAL → RESOLVED. +- Single mechanical sweep; no per-file design decisions; smoke + + vitest defend against regression. +- Same precedent shape applies (Brief 4's commit `297afca` is the + reference diff). + +**Architect's responsibility at gate 1:** confirm Option B is +mechanically safe (no file in the 24 has unique pre-OPTIONS body +logic that depends on the wildcard, no file is doing a *narrow* +CORS hint that should be preserved-but-tightened rather than +deleted), or recommend Option A with explicit reasoning. Default +recommendation is B — same-origin Vercel deployment means CORS +headers serve no legitimate purpose anywhere on this API surface. + +## Operator action required + +**None.** No env vars, no secrets, no infra changes. + +## Decisions to ratify with operator + +1. **Option A (narrow) vs Option B (expanded).** See Scope § + above. Parent recommends Option B; architect investigates and + ratifies. +2. **Should the OPTIONS preflight handler be replaced with a + `405 Method Not Allowed`?** Once the wildcard CORS is gone, + browsers will not send preflights to this API (same-origin + doesn't preflight). Three sub-options: + - **(a)** Delete the OPTIONS handler entirely. Method-check + at top of handler (`if (req.method !== 'POST')`) returns 405. + Same end-state as Brief 4 did to login + register. + - **(b)** Keep the OPTIONS handler, return 405. Slightly + friendlier to any future direct-CLI callers, but contradicts + the "no special-case OPTIONS" simplification. + - **(c)** Keep the OPTIONS handler, return 204. Strictly + correct per RFC 7231 for an empty success response. + Recommend (a). Matches Brief 4 precedent. Trivial to revisit + if a real cross-origin caller ever lands. +3. **`pages/api/auth/verify.js` is a GET endpoint** — its + `Allow-Methods` line is `'GET, POST, PUT, DELETE, OPTIONS'` + (over-permissive). Should the method gate be tightened to + GET-only at the same time? Recommend YES — the handler + already has `if (req.method !== 'GET') return 405` at line 17, + so tightening the `Allow-Methods` line is moot once it's + deleted. No-op. +4. **Any tests need updating?** Vitest suite covers + `pages/api/auth-utils.js` and `lib/permission-middleware.js` + but NOT the per-route handlers directly. Smoke suite covers + `home`, `sign-in`, `/api/health` — none of the 24 routes + in scope are smoke-covered today, and smoke uses same-origin + so CORS removal won't affect it. Recommend NO new tests in + this convoy (deferred to a per-route handler test convoy that + doesn't exist yet). + +## Known constraints + +- **All 24 files use the IDENTICAL scaffolded pattern** (parent + spot-checked 3 of 24; architect should spot-check 5+ more to + rule out drift). Same 3-line CORS block, same OPTIONS-guard + block. Differs only in the `Allow-Methods` verb list. +- **Brief 4 of `fix-auth-bypass` (commit `297afca`)** is the + exact precedent. The shape there was: delete the comment, delete + the 3 setHeader calls, delete the if-OPTIONS block. That is the + edit to apply 24 times. +- **No lib-level change.** No need to add a "CORS helper module" + or anything else — the right answer is "no CORS at all", same as + Brief 4 settled on. +- **CI workflow `forbidden-endpoints` job** does NOT currently + forbid CORS headers. If we want a regression lock, the architect + could optionally add a new CI grep-gate. Recommend YES if Option + B is chosen (locks in the cleanup so it can't accumulate again). +- **Same-origin assumption holds** — the API and frontend live on + the same Vercel project (same domain). If that ever changes + (separate API subdomain, mobile app calling the API directly), + a real CORS layer needs to be designed at that point. This + convoy explicitly does NOT design for that future — YAGNI. + +## Acceptance criteria + +The convoy is shippable when ALL of the following hold: + +1. The targeted files (`verify.js` for Option A; all 24 for + Option B) have zero `Access-Control-Allow-Origin` references. +2. The targeted files have zero `if (req.method === 'OPTIONS')` + blocks. +3. `npm run lint` exit code matches baseline (still 128 problems + per `fix-lint-baseline`; do NOT regress). +4. `npm run test:run` (vitest) still passes 21/21 (no regression). +5. `npm run test:smoke` (via CI on the PR) still passes 3/3 against + the Vercel preview — proves login + verify + /api/health flow + end-to-end after the CORS removal. +6. `git grep -nE "Access-Control-Allow-Origin" -- 'pages/api/**'` + returns zero matches (Option B) OR exactly N-1 matches where + N=23 (Option A). +7. If Option B AND the architect picks "add CI regression lock": + `.github/workflows/ci.yml`'s `forbidden-endpoints` job (or a + new `forbidden-cors-headers` job) fails when any new + `Access-Control-Allow-Origin` is reintroduced. + +## Anything flagged but not acted on (in advance) + +- **A real CORS layer for a future mobile / 3rd-party API + consumer.** Out of scope. If/when needed, design from scratch + (probably as middleware) rather than re-scaffolding wildcards. +- **`cors-sweep-all-routes` follow-up** — only relevant if Option + A is chosen. Pre-queue the entry in ship-readiness's Queued + convoys section if Option A wins. +- **API-route handler unit tests** — none of the 24 files have + vitest coverage today. Adding handler-level tests for each is + a separate convoy (probably `fill-vitest-handler-coverage`). +- **OPTIONS / CORS via Next.js middleware** — `middleware.js` + doesn't exist. Adding a middleware layer to enforce same-origin + is an over-engineered fix for "remove unnecessary headers"; + YAGNI. Note for posterity in case a future agent considers it.