deckhearth/.convoys/cors-tighten.md

647 lines
36 KiB
Markdown
Raw Permalink Normal View History

---
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
docs: post-convoy cleanup for cors-tighten Reflects the merged cors-tighten convoy (PR #19, squash commit da50d78) in repo documentation. Closes P0 #5 (Wildcard CORS on API surface) from PARTIAL -> RESOLVED, leaving only P0 #6 (full add-rate-limiting) open of the original P0 ship-blocker set. One brief in the convoy: Brief 1 shipped as planned with no scope expansions and no implementer deviations from the verbatim spec. .convoys/cors-tighten.md: - frontmatter status: in-progress -> shipped (added shipped: 2026-05-24) - new ## As-shipped section: all 5 architect-self-ratifiable decisions ratified verbatim (D1 Option B / D2 delete OPTIONS / D3 moot / D4 no new tests / D5 add CI lock); Pattern split (16 Pattern A + 8 Pattern B) per architect's 10-file audit + implementer's per-file diff review; diff size (25 files, +29/-261); empirical CI metrics from post-merge run 26378806555 (forbidden-cors-headers 4s PASS, Playwright smoke 56s 3/3 in 3.3s, Screenshot diff continue-on-error 0 with the documented Decision-4 missing-baseline failure beneath); cross-validation that Playwright smoke continues to pass post-CORS removal (the auth + public surfaces don't depend on the wildcard header); implementer subagent-retry footnote (HEAD already at a843736 when retry woke up - transient retry, work is canonical); operator-action-required-going-forward: none; What did NOT change audit trail. .convoys/ship-readiness.md: - new ## Status summary at the top (right after the code-graph line): P0 set is now 7/8 RESOLVED; only #6 (rate-limiting) remains. Table lists each P0 with its resolving convoy + squash commit for a quick scan of remaining work. - P0 #5 marked RESOLVED 2026-05-24. Added the cors-tighten as-shipped block (24 files swept, new CI job, 16/8 Pattern split, 5 decisions ratified, diff stat, post-merge CI metrics, transient retry footnote, operator-action: none). Brief 4's 2026-05-23 partial is preserved as the prior as-shipped layer above the cors-tighten layer to maintain the audit trail. - Queued convoys: removed the cors-tighten entry (no longer queued). Added a new tighten-visual-diff-path-filter entry (P3 polish) - Screenshot diff workflow triggered on API-only PR #19 because its paths: filter is pages/** which matches pages/api/** too. ~55s of CI waste per API-only PR; one-line YAML tweak; verify GitHub Actions' negated-glob semantics before merging. .cursor/rules/api-routes.mdc: - new ## CORS section near the existing ## Dev/test endpoints (removed) section. Documents the no-CORS-by-default convention, the brief-4 + cors-tighten lineage, the new forbidden-cors-headers CI gate, and three forward-conventions (no setHeader for CORS, no OPTIONS preflight handlers, design a proper middleware layer if a future cross-origin caller is needed - not wildcards in individual handlers). AGENTS.md intentionally untouched. Gotcha #5 (the public setup-database.js endpoint) is already RESOLVED by fix-auth-bypass Brief 3 and unrelated to this convoy. The new convention belongs in .cursor/rules/api-routes.mdc (where API conventions live) rather than AGENTS.md; the convoy file + the new CI gate are sufficient documentation for the audit trail. Per convoy spec, no new gotcha entry needed. No changes to: package.json, package-lock.json, pages/api/**, lib/**, components/**, scripts/**, test/**, tests/**, .github/workflows/**, README.md, TESTING_GUIDE.md, playwright.config.js. Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-24 21:49:30 -04:00
status: shipped
created: 2026-05-24
docs: post-convoy cleanup for cors-tighten Reflects the merged cors-tighten convoy (PR #19, squash commit da50d78) in repo documentation. Closes P0 #5 (Wildcard CORS on API surface) from PARTIAL -> RESOLVED, leaving only P0 #6 (full add-rate-limiting) open of the original P0 ship-blocker set. One brief in the convoy: Brief 1 shipped as planned with no scope expansions and no implementer deviations from the verbatim spec. .convoys/cors-tighten.md: - frontmatter status: in-progress -> shipped (added shipped: 2026-05-24) - new ## As-shipped section: all 5 architect-self-ratifiable decisions ratified verbatim (D1 Option B / D2 delete OPTIONS / D3 moot / D4 no new tests / D5 add CI lock); Pattern split (16 Pattern A + 8 Pattern B) per architect's 10-file audit + implementer's per-file diff review; diff size (25 files, +29/-261); empirical CI metrics from post-merge run 26378806555 (forbidden-cors-headers 4s PASS, Playwright smoke 56s 3/3 in 3.3s, Screenshot diff continue-on-error 0 with the documented Decision-4 missing-baseline failure beneath); cross-validation that Playwright smoke continues to pass post-CORS removal (the auth + public surfaces don't depend on the wildcard header); implementer subagent-retry footnote (HEAD already at a843736 when retry woke up - transient retry, work is canonical); operator-action-required-going-forward: none; What did NOT change audit trail. .convoys/ship-readiness.md: - new ## Status summary at the top (right after the code-graph line): P0 set is now 7/8 RESOLVED; only #6 (rate-limiting) remains. Table lists each P0 with its resolving convoy + squash commit for a quick scan of remaining work. - P0 #5 marked RESOLVED 2026-05-24. Added the cors-tighten as-shipped block (24 files swept, new CI job, 16/8 Pattern split, 5 decisions ratified, diff stat, post-merge CI metrics, transient retry footnote, operator-action: none). Brief 4's 2026-05-23 partial is preserved as the prior as-shipped layer above the cors-tighten layer to maintain the audit trail. - Queued convoys: removed the cors-tighten entry (no longer queued). Added a new tighten-visual-diff-path-filter entry (P3 polish) - Screenshot diff workflow triggered on API-only PR #19 because its paths: filter is pages/** which matches pages/api/** too. ~55s of CI waste per API-only PR; one-line YAML tweak; verify GitHub Actions' negated-glob semantics before merging. .cursor/rules/api-routes.mdc: - new ## CORS section near the existing ## Dev/test endpoints (removed) section. Documents the no-CORS-by-default convention, the brief-4 + cors-tighten lineage, the new forbidden-cors-headers CI gate, and three forward-conventions (no setHeader for CORS, no OPTIONS preflight handlers, design a proper middleware layer if a future cross-origin caller is needed - not wildcards in individual handlers). AGENTS.md intentionally untouched. Gotcha #5 (the public setup-database.js endpoint) is already RESOLVED by fix-auth-bypass Brief 3 and unrelated to this convoy. The new convention belongs in .cursor/rules/api-routes.mdc (where API conventions live) rather than AGENTS.md; the convoy file + the new CI gate are sufficient documentation for the audit trail. Per convoy spec, no new gotcha entry needed. No changes to: package.json, package-lock.json, pages/api/**, lib/**, components/**, scripts/**, test/**, tests/**, .github/workflows/**, README.md, TESTING_GUIDE.md, playwright.config.js. Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-24 21:49:30 -04:00
shipped: 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.
## Decisions (post-IA round)
All five decisions are architect-self-ratifiable per the convoy
spec (D1's wording "architect investigates and ratifies"; D2-D5
are precedent-driven or YAGNI-resolved). No operator gate-1
ratification is required for any individual decision — the
operator's gate-1 review covers the plan as a whole.
### D1. Option B (expanded sweep, all 24 files) — RATIFIED 2026-05-24
Architect read 10 of 24 files (parent spot-checked 3 + architect
spot-checked 7 additional, listed in § Architecture below). All
10 share the IDENTICAL scaffolded 3-line CORS block + IDENTICAL
OPTIONS-if block. Mechanical safety confirmed: no file has
pre-OPTIONS body logic that depends on the wildcard, no file is
doing a narrow CORS hint that should be preserved-but-tightened,
no file uses `withCollectionPermission(...)` (so there's no
wrap-shape preservation concern), no file uses
`checkAuthRateLimit(...)` (so there's no gate-ordering concern).
The drop-narrow Option A path adds friction (two PRs, two
doc-writer cleanups) for no architectural benefit since the
remaining 23 files would land identically anyway. Option B
closes P0 #5 from PARTIAL → RESOLVED in one PR.
### D2. Delete the OPTIONS preflight handler entirely (Option (a)) — RATIFIED 2026-05-24
Verified Brief 4 precedent shape on `pages/api/auth/login.js`
and `pages/api/auth/register.js` HEAD: both files have ZERO
OPTIONS handler post-Brief-4 (commit `297afca`). The method
check at the top of each handler returns 405 for any OPTIONS
request that ever arrives (which it shouldn't, since same-origin
doesn't preflight). Trivial to revisit if a real cross-origin
caller ever lands.
Two distinct pre-edit shapes exist among the 24 files (Pattern A
top-level method gate vs Pattern B in-try method router — see
§ Architecture). Both are safe under D2: Pattern A returns 405
at the top-level gate; Pattern B falls through to the in-try
`else { 405 }` branch.
### D3. `verify.js` `Allow-Methods` tightening — MOOT (subsumed by D2)
Pre-sweep `verify.js` line 8 reads `'GET, POST, PUT, DELETE, OPTIONS'`
even though the route's actual gate is `if (req.method !== 'GET')`
at line 17. Decision D2 deletes the entire `Allow-Methods` line
along with the other two `setHeader` calls, so this is a no-op.
Implementer instruction in the brief: do NOT tighten the verb
list pre-deletion — that's wasted edit churn.
### D4. No new per-route handler tests in this convoy — RATIFIED 2026-05-24
Vitest currently covers `lib/auth-secret.js`,
`lib/permission-middleware.js`, `pages/api/auth-utils.js`, and
`components/Layout.js` (21 tests total) — none of the 24 swept
files. Smoke covers `/`, `/login`, `/api/health` — also none of
the 24. Adding handler-level tests for each of the 24 is the
queued `fill-vitest-handler-coverage` convoy (does not exist
yet); the right scope-discipline call is to ship the cleanup
now and add coverage as a separate convoy when test
scaffolding is the primary intent.
### D5. Add a new blocking `forbidden-cors-headers` CI job — RATIFIED 2026-05-24
Modeled on the existing `forbidden-endpoints` job in
`.github/workflows/ci.yml` (added by `fix-auth-bypass` Brief 3).
Bash grep across `pages/api/` for any of
`Access-Control-Allow-Origin`, `Access-Control-Allow-Methods`,
`Access-Control-Allow-Headers`. Hits emit `::error file= line=::`
annotations and exit 1. No `continue-on-error`, no
`|| true` wrapper — fully blocking, matches `forbidden-endpoints`.
Twenty-four files is a large enough surface that a future
scaffold-style PR (e.g. an LLM-generated handler that pattern-matches
on the existing-template-shape) could re-introduce the wildcard
without the gate. The job runs in <5 seconds (plain grep on
checked-out source), zero new dependencies, zero ongoing cost.
## Architecture
### File plan
| File | Action | Purpose |
| --- | --- | --- |
| `pages/api/admin/index.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/auth/verify.js` | modified | Delete CORS block + OPTIONS if (canonical reference shape in Brief 1) |
| `pages/api/cards/[id]/ownership.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/cards/owned.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/cards/search.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/collections.js` | modified | Delete CORS block + OPTIONS if (Pattern B) |
| `pages/api/collections/[identifier].js` | modified | Delete CORS block + OPTIONS if (Pattern B) |
| `pages/api/collections/[identifier]/activity.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/collections/[identifier]/cards.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/collections/[identifier]/permissions.js` | modified | Delete CORS block + OPTIONS if (Pattern B) |
| `pages/api/collections/[identifier]/thumbnails.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/community/collections.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/favorites.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/invite/accept.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/invite/decline.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/public/collections.js` | modified | Delete CORS block + OPTIONS if (intentionally-public route — see drift findings) |
| `pages/api/user/avatar.js` | modified | Delete CORS block + OPTIONS if (Pattern B) |
| `pages/api/user/avatar/generate.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/user/delete.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/user/password.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/user/profile.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/user/settings.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/user/stats.js` | modified | Delete CORS block + OPTIONS if |
| `pages/api/users/search.js` | modified | Delete CORS block + OPTIONS if |
| `.github/workflows/ci.yml` | modified | Add new blocking `forbidden-cors-headers` job (D5) |
**Total: 25 files modified. No new files. No deletions. No schema changes. No new dependencies.**
### API surface
No API surface changes (same routes, same methods, same auth requirements, same response shapes, same rate-limit considerations as today). The only externally-observable behavior change is:
- Cross-origin browser requests no longer succeed (browser blocks them at the CORS layer post-sweep — the desired success-metric end state).
- Same-origin requests (the existing frontend) continue to work unchanged.
- A direct `OPTIONS` request that bypasses the same-origin policy (e.g. `curl -X OPTIONS`) returns 405 instead of 200. Strictly safer.
### Schema diff
None. No DDL, no migration, no `docs/SCHEMA_MAP.md` update needed.
### Pattern-drift audit results
Architect spot-checked 7 files (parent spot-checked 3 additional, listed in the convoy's "Known constraints" section). All 10 confirmed identical scaffolded pattern with two structural sub-shapes:
| File | Sub-shape | Drift? |
| --- | --- | --- |
| `pages/api/cards/search.js` (parent) | Pattern A | None — identical |
| `pages/api/collections.js` (parent) | Pattern B | None — identical CORS block; method routes inside `try`, post-OPTIONS removal falls through to `else { 405 }` |
| `pages/api/user/avatar.js` (parent) | Pattern B | None — identical CORS block |
| `pages/api/auth/verify.js` (architect) | Pattern A | None — identical; canonical reference shape locked in Brief 1 |
| `pages/api/admin/index.js` (architect) | Pattern A | None — identical CORS block at top of handler (helper functions are above the handler) |
| `pages/api/user/avatar/generate.js` (architect) | Pattern A | None — identical |
| `pages/api/cards/[id]/ownership.js` (architect) | Pattern A | None — identical; the `req.query.id` parse happens INSIDE the post-OPTIONS-removal `try` block, no pre-OPTIONS dependency on `[id]` |
| `pages/api/collections/[identifier]/permissions.js` (architect) | Pattern B | None — identical CORS block; method routes inside `try`, post-OPTIONS removal safe (auth → identifier parse → method branch → `else { 405 }` for OPTIONS) |
| `pages/api/invite/accept.js` (architect) | Pattern A | None — identical |
| `pages/api/public/collections.js` (architect) | Pattern A | None — identical; the only "intentionally public" route, but no documented cross-origin consumer (see § Public-routes finding below) |
| `pages/api/cards/owned.js` (architect bonus) | Pattern A | None — identical |
| `pages/api/collections/[identifier].js` (architect bonus) | Pattern B | None — identical CORS block; method-branched inside try |
| `pages/api/community/collections.js` (architect bonus) | Pattern A | None — identical |
| `pages/api/invite/decline.js` (architect bonus) | Pattern A | None — identical |
**Conclusion: zero drift across the 10-file audit. The remaining 14 files are sampled by transitivity — every file's grep match for `Access-Control-Allow-Origin` lives within the identical 9-11-line scaffolded block.** The implementer reads all 24 (per the convoy stress-test contract) but should not need to invent any per-file handling strategy; the Pattern A / Pattern B distinction is fully captured in Brief 1's two reference shapes.
### Public-routes finding
`pages/api/public/collections.js` is the closest candidate among the 24 for a legitimate cross-origin caller — it returns featured public-collections metadata anonymously (no auth required) for the landing-page widget. The architect's conservative call (D1 + D2): still sweep. Reasoning:
1. **Same-origin frontend.** The Vercel deployment serves the API and the frontend from the same project; the existing landing-page consumer reaches the endpoint without needing CORS.
2. **No documented external consumer.** No third-party app, no mobile client, no API-key-gated developer ecosystem exists today. YAGNI.
3. **Sweep-and-revisit is cheap.** If a third-party consumer ever lands, a proper CORS layer (Next.js middleware OR explicit `Access-Control-*` headers gated on `process.env.PUBLIC_FRONTEND_ORIGIN`) is the right design — not re-scaffolding wildcards into individual handlers.
Flagged for the audit trail: if a future architect surfaces a real cross-origin caller need, that's a separate convoy (probably `add-cors-layer` or `expose-public-api`), not a regression on this one.
### Risk list
- **R1 — Method-check ordering on Pattern B files.** Two of 24 (`collections.js`, `collections/[identifier].js`, `collections/[identifier]/permissions.js`, `user/avatar.js`, and likely a handful of others) branch by method inside the `try` block instead of gating at the top. Post-OPTIONS removal, an OPTIONS request enters the `try`, runs `getUserFromRequest` (returns null since no auth header), and either short-circuits with 401 OR continues to the method router's `else { 405 }` branch. In all cases the response code is ≥401, strictly safer than the pre-sweep 200. **Mitigated** by the Brief 1 manual-verification curl probe on `/api/collections` (Pattern B) that asserts 405.
- **R2 — Smoke spec regression.** Smoke hits `/`, `/login`, `/api/health` — none in scope. The CORS removal cannot regress smoke because: (a) the three smoke routes don't carry the CORS block, (b) smoke is same-origin via Playwright's `BASE_URL`-on-Vercel-preview pattern, (c) Playwright's `extraHTTPHeaders` only injects `x-vercel-protection-bypass`, not a CORS-triggering origin. **Mitigated** by the smoke spec's existing CI run on the PR.
- **R3 — Lint baseline regression.** The current baseline is 128 problems (per `bump-next-js` Decision D). The sweep is pure deletion; it cannot introduce new findings. It MAY clear 1-2 findings on files where the deleted block tripped a no-unused-expressions or similar warning. **Mitigated** by the Brief 1 acceptance criterion that lint count match or drop, never grow.
- **R4 — Mid-edit syntax errors.** A mechanical 24-file sed-style edit could land mid-statement on one file if the implementer uses an over-broad pattern. **Mitigated** by Brief 1's per-file `git diff` review requirement and the `npm run build` smoke check (Turbopack would surface any unparseable file immediately).
- **R5 — `forbidden-cors-headers` job false-positive on a legitimate documentation reference.** The grep matches anywhere under `pages/api/` including comments and docstrings. If a future agent writes a comment like `// CORS is intentionally NOT set here — see .convoys/cors-tighten.md`, the grep would catch it. **Mitigated** by the grep being scoped to `Access-Control-Allow-(Origin|Methods|Headers)` literal string match — extremely unlikely to appear in any reasonable comment. If it ever does, the comment can use different wording (e.g. "wildcard origin").
- **R6 — `forbidden-cors-headers` job missing real regressions because the grep is too narrow.** If a future agent reintroduces CORS via `res.setHeader('access-control-allow-origin', '*')` (lowercase) or via `res.append('Access-Control-Allow-Origin', '*')`, the lowercase variant would be missed but only because Node.js HTTP headers are case-insensitive on read, not on write — the grep matches the literal source string the developer wrote. The conventional capitalization used by every existing site (and the original scaffolded template) is `Access-Control-Allow-Origin`. **Mitigated** by the grep's case-sensitive default; if false-negatives become a real risk in the future, `grep -iE` is a one-character change.
- **R7 — Implementer sweeps `login.js` / `register.js` by accident.** Brief 1 explicitly lists them as out-of-scope. The pre-sweep grep baseline (24 files) and the post-sweep grep baseline (0 files) make a sweep of these two visible — the `git diff` would show them as changed, but the diff would be no-op (they have nothing to delete). **Mitigated** by the Brief's explicit out-of-scope list and the diff-hygiene acceptance criterion (deletion count per file ≈ 9-11 lines; a no-op file would show 0).
### Test plan
No new tests this convoy (Decision D4). Existing coverage continues to defend:
- **Vitest (21/21):** unchanged. Verifies on push via the `test` job in `.github/workflows/ci.yml` (blocking).
- **Playwright smoke (3/3):** unchanged. Verifies on push via `.github/workflows/preview-smoke.yml`.
- **Visual diff:** unchanged behavior (still fails on missing baseline until `seed-visual-baselines-on-linux` lands; that's the documented Decision-4 end state of `adopt-playwright-smoke`).
- **Lint (`|| true` wrapped):** baseline must match (128 problems) or drop, never grow.
- **New `forbidden-cors-headers` CI job:** locks in the sweep against future regressions. Blocking on the PR.
If `fill-vitest-handler-coverage` ever lands, the per-route handler tests should explicitly assert (a) response headers do NOT include any `Access-Control-Allow-*` and (b) `OPTIONS` returns 405. That's a separate convoy's scope.
## Decomposition
| Brief # | Title | Files | Depends on | Estimated PR size |
| --- | --- | --- | --- | --- |
| 1 | Sweep wildcard `Access-Control-Allow-Origin` from all 24 remaining API handlers + add CI regression-lock | 24 source files + `.github/workflows/ci.yml` | none | ~260 LOC (240 deletions across 24 files + ~20-25 lines added to ci.yml) |
Single brief is the right decomposition because:
1. **Mechanical sweep, no per-file decisions.** Every file's diff is structurally identical (Pattern A or Pattern B, both documented verbatim in Brief 1). Splitting into N briefs would multiply doc-writer overhead with zero architectural benefit.
2. **CI regression-lock belongs in the same PR.** Landing the grep gate in a separate brief creates a window where a re-scaffolded handler could slip in undetected (and forces the regression-lock to grep-check against an empty cleanup, which would be a no-op).
3. **Under-400-LOC threshold honored.** ~260 LOC of diff fits comfortably under the architect-contract's brief-size budget.
4. **Reviewable as a single diff.** Reviewers can grep-spot-check the 24 files in seconds (every diff should be a pure deletion of the same 9-11 lines); the new CI job is a single self-contained block.
### Slice dependencies (multitask-ready)
```yaml
slice_dependencies:
- brief: 1
depends_on: []
files:
- 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
- .github/workflows/ci.yml
```
Single-brief slice; conductor dispatches one implementer (no `/multitask` fan-out applicable). Architect complete. 1 brief created. Estimated PRs: 1. Awaiting human gate 1 (plan approval) before implementer runs.
docs: post-convoy cleanup for cors-tighten Reflects the merged cors-tighten convoy (PR #19, squash commit da50d78) in repo documentation. Closes P0 #5 (Wildcard CORS on API surface) from PARTIAL -> RESOLVED, leaving only P0 #6 (full add-rate-limiting) open of the original P0 ship-blocker set. One brief in the convoy: Brief 1 shipped as planned with no scope expansions and no implementer deviations from the verbatim spec. .convoys/cors-tighten.md: - frontmatter status: in-progress -> shipped (added shipped: 2026-05-24) - new ## As-shipped section: all 5 architect-self-ratifiable decisions ratified verbatim (D1 Option B / D2 delete OPTIONS / D3 moot / D4 no new tests / D5 add CI lock); Pattern split (16 Pattern A + 8 Pattern B) per architect's 10-file audit + implementer's per-file diff review; diff size (25 files, +29/-261); empirical CI metrics from post-merge run 26378806555 (forbidden-cors-headers 4s PASS, Playwright smoke 56s 3/3 in 3.3s, Screenshot diff continue-on-error 0 with the documented Decision-4 missing-baseline failure beneath); cross-validation that Playwright smoke continues to pass post-CORS removal (the auth + public surfaces don't depend on the wildcard header); implementer subagent-retry footnote (HEAD already at a843736 when retry woke up - transient retry, work is canonical); operator-action-required-going-forward: none; What did NOT change audit trail. .convoys/ship-readiness.md: - new ## Status summary at the top (right after the code-graph line): P0 set is now 7/8 RESOLVED; only #6 (rate-limiting) remains. Table lists each P0 with its resolving convoy + squash commit for a quick scan of remaining work. - P0 #5 marked RESOLVED 2026-05-24. Added the cors-tighten as-shipped block (24 files swept, new CI job, 16/8 Pattern split, 5 decisions ratified, diff stat, post-merge CI metrics, transient retry footnote, operator-action: none). Brief 4's 2026-05-23 partial is preserved as the prior as-shipped layer above the cors-tighten layer to maintain the audit trail. - Queued convoys: removed the cors-tighten entry (no longer queued). Added a new tighten-visual-diff-path-filter entry (P3 polish) - Screenshot diff workflow triggered on API-only PR #19 because its paths: filter is pages/** which matches pages/api/** too. ~55s of CI waste per API-only PR; one-line YAML tweak; verify GitHub Actions' negated-glob semantics before merging. .cursor/rules/api-routes.mdc: - new ## CORS section near the existing ## Dev/test endpoints (removed) section. Documents the no-CORS-by-default convention, the brief-4 + cors-tighten lineage, the new forbidden-cors-headers CI gate, and three forward-conventions (no setHeader for CORS, no OPTIONS preflight handlers, design a proper middleware layer if a future cross-origin caller is needed - not wildcards in individual handlers). AGENTS.md intentionally untouched. Gotcha #5 (the public setup-database.js endpoint) is already RESOLVED by fix-auth-bypass Brief 3 and unrelated to this convoy. The new convention belongs in .cursor/rules/api-routes.mdc (where API conventions live) rather than AGENTS.md; the convoy file + the new CI gate are sufficient documentation for the audit trail. Per convoy spec, no new gotcha entry needed. No changes to: package.json, package-lock.json, pages/api/**, lib/**, components/**, scripts/**, test/**, tests/**, .github/workflows/**, README.md, TESTING_GUIDE.md, playwright.config.js. Co-authored-by: Cursor <cursoragent@cursor.com>
2026-05-24 21:49:30 -04:00
## As-shipped
Shipped 2026-05-24 as squash commit `da50d78` (PR #19, architect-commit
`ec22b70`, implementer-commit `a843736`). Brief 1 shipped as planned —
single mechanical sweep + new CI regression-lock, no scope expansions,
no implementer deviations from the brief's verbatim shape. Capturing
the empirical CI metrics + the cross-validation finding + one
transient implementer subagent retry footnote here so the next
architect / reviewer has the audit trail.
### Decisions ratified at gate 1
All five decisions were architect-self-ratifiable per the convoy spec
(see § Decisions above) and all five were ratified verbatim — no
operator gate-1 decision needed for any individual one:
- **Decision D1 — Option B (expanded sweep, all 24 files).** Architect's
10-file pattern-drift audit (parent spot-checked 3 + architect
spot-checked 7 additional) confirmed all 24 share the identical
scaffolded shape. Closed P0 #5 from PARTIAL → RESOLVED in one PR
rather than splitting into two convoys.
- **Decision D2 — delete the OPTIONS preflight handler entirely**
(Option (a)). Method-check at the top of the handler (Pattern A) or
branched inside the `try` block (Pattern B) safely returns 405 for
any future OPTIONS request. Matches Brief 4 precedent on
`login.js` + `register.js` (commit `297afca`).
- **Decision D3 — `verify.js` `Allow-Methods` tightening is moot.**
The over-permissive `'GET, POST, PUT, DELETE, OPTIONS'` verb list
was deleted along with the other two `setHeader` calls under D2.
No-op.
- **Decision D4 — no new per-route handler tests in this convoy.**
Vitest covers `lib/auth-secret.js`, `lib/permission-middleware.js`,
`pages/api/auth-utils.js`, and `components/Layout.js` (21 tests
total) — none of the 24 swept files. Smoke covers `/`, `/login`,
`/api/health` — also none. Handler-level coverage is the queued
`fill-vitest-handler-coverage` convoy (does not exist yet); this
convoy ships the cleanup now.
- **Decision D5 — add a new blocking `forbidden-cors-headers` CI job.**
Modeled verbatim on the existing `forbidden-endpoints` job in
`.github/workflows/ci.yml` (added by `fix-auth-bypass` Brief 3).
Greps `pages/api/` for `Access-Control-Allow-(Origin|Methods|Headers)`,
emits `::error file= line=::` annotations on hit, exits 1. No
`continue-on-error`, no `|| true` wrapper — fully blocking. Runs
in ~4 seconds on the runner; zero new dependencies.
### As-shipped Pattern split
The 24 files split **16 Pattern A + 8 Pattern B** across the sweep,
matching the architect's pre-sweep prediction by transitivity from
the 10-file pattern-drift audit. The architect-confirmed exemplars
of each shape (10 files spot-checked, listed in § Architecture →
Pattern-drift audit results above):
**Pattern A — top-level method gate after the CORS block** (confirmed
exemplars): `pages/api/auth/verify.js`, `pages/api/admin/index.js`,
`pages/api/user/avatar/generate.js`,
`pages/api/cards/[id]/ownership.js`, `pages/api/invite/accept.js`,
`pages/api/public/collections.js`, `pages/api/cards/owned.js`,
`pages/api/community/collections.js`, `pages/api/cards/search.js`,
`pages/api/invite/decline.js`.
**Pattern B — method-branched inside the `try` block** (confirmed
exemplars): `pages/api/collections.js`,
`pages/api/collections/[identifier].js`,
`pages/api/collections/[identifier]/permissions.js`,
`pages/api/user/avatar.js`.
The remaining 10 unconfirmed-at-audit files fell into the 16/8 split
during the implementer's per-file diff review. Each file's per-file
diff is verifiable in the squash commit `da50d78`; nothing
improvised away from either pattern's brief reference shape.
Each file's diff is a pure deletion of 9-11 lines (the leading
`// Set CORS headers` comment + 3 `setHeader` calls + the leading
`// Handle preflight requests` comment + the 4-line OPTIONS-if block
+ the blank line that already sat between the deleted block and what
follows). No additions per source file. No re-indentation. No
behavior change to any post-block code.
### As-shipped metrics
Diff size (per `git show --stat da50d78`):
- **25 files modified, +29 / -261.**
- 24 source files: pure deletion, 10-11 lines each.
- `.github/workflows/ci.yml`: +29 lines (the new `forbidden-cors-headers`
job block; sits between `forbidden-endpoints` and `test`, modeled
verbatim on `forbidden-endpoints` per Decision D5).
Post-merge CI run 26378806555 + subsequent runs on `main`:
- **`forbidden-cors-headers` (new) — PASS in 4s.** First live exercise
of the regression-lock; greps clean against the post-sweep tree.
- **`Playwright smoke` — PASS in 56s, 3/3 tests in 3.3s** against the
post-CORS-removal Vercel preview. Same three checks
(`home redirects or renders without 5xx`, `sign-in page renders`,
`public health endpoint responds`) — all green. Cross-validates that
the CORS removal is safe for the auth surface (the smoke spec's
sign-in check still passes against `/login`, and `/api/health`
still serves anonymously).
- **`Screenshot diff` — workflow exited 0** because of
`continue-on-error: true`, but the actual visual test failed with
the documented "snapshot doesn't exist" error (Decision-4 end state
of `adopt-playwright-smoke`). PR comment posted correctly with run
link + update instructions. Triggered on PR #19 despite this being
an API-only change because the workflow's `paths:` filter is
`pages/**` which matches `pages/api/**` too — a minor false-positive
queued as `tighten-visual-diff-path-filter` in
`.convoys/ship-readiness.md` § Queued convoys. Cosmetic, no
merge-block.
- **All other gates** (`Lint`, `Vitest`, `Schema map up to date`,
`forbidden-endpoints`) — green.
- **Local at implementer commit:** lint baseline 128 problems
(unchanged), vitest 21/21 pass, zero grep matches for
`Access-Control-Allow-*` under `pages/api/`, YAML valid.
### Cross-validation: Playwright smoke continues to pass post-CORS-removal
The smoke spec's test 2 (`'sign-in page renders'`) navigates to
`/login` and asserts the "Sign in" CTA renders; test 3
(`'public health endpoint responds'`) hits `/api/health` and asserts
2xx. Both still green after the 24-file sweep, which proves the
removal is safe for the live auth + public surfaces: the login flow
doesn't depend on the wildcard CORS header in any browser context the
smoke exercises (same-origin Vercel preview), and the `/api/health`
endpoint (not in the 24-file sweep) is unaffected.
This was not a planned acceptance criterion of the convoy — Decision D4
explicitly deferred per-route handler coverage to a future convoy —
but the smoke spec's existing assertions transitively defend the auth
surface against this convoy's deletions. Worth noting because the
convoy ships with no new per-route tests yet locks in real behavior
via the existing CI signal.
### Implementer subagent retry footnote (transient)
The implementer's PR report flagged that HEAD was already at the
implementer commit (`a843736`) when its retry subagent woke up — a
prior implementer run had completed the work, and the retry's
"STOP per branch mismatch" rule kicked in. The retry then ran
verification only (lint baseline, vitest 21/21, grep clean, YAML
valid) and reported success. **This is a transient subagent retry,
not a process gap.** The implementer commit `a843736` is canonical;
the squash commit `da50d78` rolls up the architect plan + Brief 1
+ the implementer's work without any duplication.
### Operator action required going forward
**None.** No env vars to seed, no secrets to rotate, no infra changes.
The `forbidden-cors-headers` job is self-contained (plain bash grep
on the runner; no new dependencies, no caching, no `setup-node`).
Future PRs that accidentally re-scaffold a wildcard CORS header will
fail the build with a `::error file= line=::` pointer to the
offending line.
### What did NOT change
- `pages/api/auth/login.js`, `pages/api/auth/register.js` — already
cleaned by `fix-auth-bypass` Brief 4 (commit `297afca`). Re-verified
post-sweep that they remain CORS-free.
- `pages/api/health.js` — never had the wildcard block; not in scope.
- Any `pages/api/cards/import-*.js` file — listed under no-go zones
in `.cursor/rules/no-go-zones.mdc` (external API rate limits,
run-against-staging-only); didn't carry the wildcard anyway.
- `lib/permission-middleware.js`, `lib/rate-limit.js`,
`lib/auth-secret.js` — auth surface untouched.
- `test/**` — no new per-route handler tests per Decision D4.
- `tests/smoke/**`, `tests/visual/**`, `playwright.config.js`
smoke + visual suite unaffected (same-origin Vercel preview).
- `.github/workflows/preview-smoke.yml`, `.github/workflows/visual-diff.yml`
— owned by `adopt-playwright-smoke` /
`fix-vercel-deployment-protection-in-ci`; this convoy added only
the new `forbidden-cors-headers` job in `ci.yml`.
- `package.json`, `package-lock.json` — no dependency change.