`REVIEW-2026-05-04.md`

71 lines
# animals — overnight debate-team review (2026-05-04)

Code-reviewer + architect-reviewer parallel run. **6 patches applied** (4 P0 security). 1 P0 deferred (helmet — touches package.json). Architecture roadmap captured.

## P0 patches APPLIED (local — NOT deployed)

| # | File:line | Issue | Fix |
|---|---|---|---|
| 1 | `src/server/index.js:975-1024` | **`/admin`, `/admin/flags`, `/admin/flags/:imageId/resolve` were UNAUTHENTICATED.** Routes registered DIRECTLY on `app` after `app.use('/admin', adminPitch)` mount, bypassing the router's auth middleware. Anyone could read leads + upgrade-orders queues, POST `marked_dead` to kill any image. | Added `requireAdminInline` middleware (mirrors admin_pitch.js's gate, checks ADMIN_EMAIL env), applied to all 3 routes with `attachUser` |
| 2 | `src/server/dog_friends.js:48` | `/friends/suggest/:petId` returned `home_zip`, `bio`, `hero_image_url` of every open-to-friends dog with no auth. Anyone enumerating petIds = harvested PII. | Added `requireUser + requirePetOwner` middleware that loads pet, verifies `pet.app_user_id === req.user.id` |
| 3 | `src/server/dog_friends.js:100` | `/friends/requests/:petId` same issue — exposed other dogs' `home_zip` to anyone | Same `requireUser + requirePetOwner` gate |
| 4 | `src/server/index.js:967-971` | `/ad/click/:creativeId` redirected to DB-controlled `cta_url` with no validation — open redirect / `javascript:` XSS if any ad row poisoned | Validate as `https:` or `http:` URL via `new URL()`; 400 on parse failure or non-http(s) scheme |
| 5 | `src/server/index.js:472,505,539` | `/sitemap.xml`, `/sitemap-images.xml`, `/robots.txt` built `base = https://${req.headers.host}` — Host-header injection let attacker poison crawler URLs to evil domain | Pinned to `process.env.PUBLIC_BASE_URL || 'https://animalsdirectory.com'` |

`node --check` passes both modified files.

## P0 deferred (touches package.json, needs Steve review)

- **No CSP / helmet anywhere.** `npm install helmet` + `app.use(helmet())` before routes. Without this, any stored XSS that lands (via `bio`, `audit.suggestions`, `biz.name`) executes with full script privileges. Same item as doctor + lawyer reviews.

## P1 / P2 deferred (track in REVIEW)

- `community.js:338-346` `/for-marketers/order` swallows `JSON.parse` errors silently — log them; validate filt.state/category against allowlist before insert.
- `dog_friends.js:13` `/register-dog` no auth, no rate limit, `bio` uncapped before INSERT — add rate limiter + `bio.slice(0,2000)`.
- `dogpark.js:106-130` WebSocket has no per-IP connection limit; `dogColor` not validated as hex (allows `slice(0,30)` of arbitrary text). Validate `/^#[0-9a-fA-F]{6}$/`.
- `index.js:615` photographer paginator interpolates `LIMIT/OFFSET` into raw SQL — switch to `LIMIT $1 OFFSET $2`.
- `community_render.js:172` `contact_email` rendered to all viewers of public listings — easily harvested. Require login or proxy via server-side form.
- `ingest/reddit.js:129` raw `selftext_html` stored in `raw_records.raw_json` — advisory; any future admin UI rendering this as HTML = stored XSS.
- `admin_pitch.js:97` George Gmail call has no Basic auth header — confirm George is localhost-only or add `Authorization: Basic`.
- No CSRF on state-changing POSTs — currently safe due to `sameSite: lax` + no `Access-Control-Allow-Origin: *`. Add csurf if either changes.

## Architecture roadmap (deferred)

**Architectural Impact: MEDIUM-HIGH.** 9-file server split is clean BUT `index.js` (1,035 LOC, 47 routes) and `render.js` (1,105 LOC, 14 exports across 5 domains) are absorbing every new feature. 3 of 5 pm2 agents are 14/30/18-line stub HTTP shells doing no real work.

### R1 — Split `render.js` by domain (1 day)
- `_layout.js` (head/nav/footer/lightbox/adSlot/leadForm/esc — currently lines 11-292)
- `home.js` (renderHome, renderAbout)
- `breeds.js` (renderBreedsIndex, renderBreed)
- `business.js` (renderCategory, renderBusiness, renderCityMap)
- `photographers.js` (renderPhotographersIndex, renderPhotographer)
- `admin.js` (renderAdmin, renderFlagQueue, renderLeadForm, renderNotFound, renderShows)

### R2 — Split `index.js` to mirror render structure
Extract `routes/api.js` (14 `/api/*` routes), `routes/breeds.js`, `routes/business.js`, `routes/photographers.js`, `routes/admin.js`, `routes/seo.js` (sitemap+robots+embed). Leave `index.js` ~150 LOC bootstrap + router mounting. Matches lawyer-directory's modular split.

### R3 — Collapse 3 pm2 stubs into `animal-agent`
- Delete `src/ingest/server.js` (14 LOC stub `POST /run/:job` returning "not implemented")
- Delete `src/audit/server.js` (30 LOC, just `spawn`s 2 CLI scripts)
- Delete `src/monetize/leads_server.js` (18 LOC wrapping one function)
- Move their routes into `agents/animal-agent/server.js` (already has `/api/queue`)
- Drop pm2 entries `animals-ingest`/`animals-audit`/`animals-leads`. Free ports 9721/9722/9723.

### R4 — Extract `directory-core` shared TS package
This is **the same recommendation that came back from doctor and lawyer reviews**. All 4 verticals (animals/doctor/lawyer/restaurant) duplicate:
- `lib/db.js` (33 LOC) — pool + many/one helpers
- `lib/geo.js` (103 LOC) — Nominatim + zip_centroids
- `lib/auth.js` (264 LOC) — claim flow, requireUser
- `monetize/{ad_engine,match_engine}.js` — ads + matching
- The "category page → business detail → claim → upgrade → leads" flow

Extract to `~/Projects/directory-core/` npm workspace. **Lawyer-directory is the right host** (only TS-native sibling). Mockup engine (`audit/generate_mockups.js` 1,192 LOC + `unique_mockups.js` + `vision_audit.js`) is the second extraction candidate.

### R5 — Fix `agents/animal-agent` boundary
Don't import `../../src/lib/db.js` from `agents/animal-agent/server.js:24-25` — cross-tree relative imports violate dependency direction. Either move agent inside `src/agents/animal-agent/` (sibling), or after R4 ships, both consume `directory-core`'s db module.

## Files touched

- `/Users/stevestudio2/Projects/animals/src/server/index.js`
- `/Users/stevestudio2/Projects/animals/src/server/dog_friends.js`