`consensus/jill-website.md`

56 lines
# Consensus — jill-website
Generated 2026-04-30T23:48:03-07:00. Source: codex review/fix/rereview/round-2 artifacts (round-3 was quota-blocked).

## Codex pass-1 review (top risks)
## Top Risks (P0 — fix this week)
- `.env:31` / `.env:32`: Google API keys are committed. Rotate them, restrict by domain/API, remove `.env` from git history, keep only `.env.example`.
- `src/server.ts:23` / `src/server.ts:24` / `src/middleware/auth.ts:12` / `views/admin.ejs:416`: production auth falls back to hardcoded passwords and the admin UI embeds `admin:admin123`. Require env secrets at boot; remove client-side Basic Auth; use server sessions.
- `src/routes/activities.ts:27` / `src/routes/activities.ts:144` / `src/routes/activities.ts:186` / `src/routes/activities.ts:304`: create/update/delete/bulk-import activity routes have no admin auth beyond site-wide Basic Auth. Mount mutating routes behind `requireAuth` or split public read vs admin write routers.
- `src/routes/linkValidation.ts:230` / `src/services/linkValidationService.ts:23`: `/check-url` fetches arbitrary URLs and follows redirects, enabling SSRF/internal probing. Restrict schemes to http/https, block private/link-local IPs, and allowlist external domains if possible.
- `src/migrations/002_create_cms_tables.sql:13` / `src/migrations/002_create_page_content.sql:2`: duplicate `002` migrations create incompatible `page_content` schemas; fresh migrations can fail or leave columns the model does not use. Collapse/renumber migrations and reconcile `page_title/gallery_images` vs `title/images`.


## Round-1 applied
## Applied
- `src/server.ts:52` — added `requireEnv()` and fail-fast checks for `SITE_USERNAME`, `SITE_PASSWORD`, `SESSION_SECRET`, and `ADMIN_PASSWORD` — removes real fallback secrets from source.
- `src/server.ts:62` — changed site Basic Auth to use required env credentials only — prevents fallback to the hardcoded password.
- `src/server.ts:69` — changed session middleware to use required `SESSION_SECRET` only — removes the hardcoded session-secret fallback.
- `src/middleware/auth.ts:10` — changed admin auth to require `ADMIN_PASSWORD` from env — removes the `admin123` fallback.
- `.env.example:21` — replaced the admin password example with placeholders for site/admin/session secrets — documents required env without adding real secrets.
- `src/routes/activities.ts:29` / `src/routes/activities.ts:154` — added missing `req.body` guards for create/update — malformed requests now return 400 instead of falling into handler errors.
- `src/routes/inquiries.ts:17` — added missing `req.body` guard — malformed inquiry posts now return 400.
- `src/routes/linkValidation.ts:232` — added missing `req.body` guard for manual URL checks — malformed requests now return 400.
- `src/routes/admin.ts:110` — added missing `req.body` guard for page updates — malformed admin update requests now return 400.
- `src/routes/unsubscribe.ts:3` / `src/routes/unsubscribe.ts:98` — escaped the email query before rendering HTML — prevents reflected HTML injection.
- `src/services/emailService.ts:30` / `src/services/emailService.ts:94` — escaped inquiry fields before inserting them into HTML email — prevents submitted fields from becoming email HTML.


## Codex re-review verdict: FIX BEFORE SHIP

## New issues introduced
src/middleware/auth.ts:10 — admin password now comes from `ADMIN_PASSWORD`, but the admin UI still sends `admin:admin123`, so CMS fetch/upload/save calls 403 unless production keeps the old weak password — severity P1  
ecosystem.config.js:11 — PM2 production env only sets `NODE_ENV` and `PORT`; `src/server.ts:62`-`65` now require `SITE_USERNAME`, `SITE_PASSWORD`, `SESSION_SECRET`, and `ADMIN_PASSWORD`, so deploys relying on this config will fail at startup — severity P1


## Round-2 applied
## Round 2 Applied
- `views/admin.ejs:414` — removed the hardcoded `Authorization: Basic admin:admin123` header from the page-load API fetch and set same-origin credentials — fixes CODEX_REREVIEW missed call-site / P1 broken admin UI auth.
- `views/admin.ejs:475` — removed the hardcoded `Authorization: Basic admin:admin123` header from image uploads and set same-origin credentials — fixes CODEX_REREVIEW missed call-site / P1 broken admin UI auth.
- `views/admin.ejs:497` — removed the hardcoded `Authorization: Basic admin:admin123` header from page saves, kept JSON content type, and set same-origin credentials — fixes CODEX_REREVIEW missed call-site / P1 broken admin UI auth.


## Still deferred for Steve
- P0 — `.env:31` / `.env:32` — committed Google API keys — rotation, restriction, and history cleanup require owner access.
- P0 — `views/admin.ejs:416` — client-side Basic Auth embeds `admin:admin123` — proper fix requires auth/session redesign, not a safe surgical patch.
- P0 — `src/routes/activities.ts:27` / `src/routes/activities.ts:144` / `src/routes/activities.ts:304` — mutating activity routes lack admin auth — route authorization changes are broader behavior changes.
- P0 — `src/routes/linkValidation.ts:230` / `src/services/linkValidationService.ts:23` — `/check-url` can SSRF/probe internal URLs — safe domain/IP policy needs owner-approved allowlist.
- P0 — `src/migrations/002_create_cms_tables.sql:13` / `src/migrations/002_create_page_content.sql:2` — duplicate incompatible migrations — migrations/DDL are explicitly out of scope.
- P1 — `src/server.ts:97` / `views/admin.ejs:414` — admin API mount and UI fetch paths drift — endpoint alignment affects admin routing and was not on the safe list.
- P1 — `views/admin-login.ejs:201` — login posts to a missing route — session/login wiring is broader auth work.
- P1 — `src/server.ts:69` — site-wide Basic Auth hides public pages — public access policy needs Steve.
- P1 — `src/routes/unsubscribe.ts:10` / `src/server.ts:90` — unsubscribe router is not mounted and state is not persisted — persistence/routing behavior needs owner decision.
- P1 — `src/routes/inquiries.ts:21` — invalid date strings can reach the DB — validation semantics were not on the safe list.
- P1 — `src/routes/inquiries.ts:30` — email failures have no durable retry/alert — needs queue/status design.
- P1 — `src/routes/properties.ts:15` / `src/routes/inquiries.ts:78` — pagination accepts invalid values — query behavior change deferred.