`REVIEW-2026-05-04.md`

75 lines
# jill-website (nosarabeachfrontrentals.com) — overnight debate-team review (2026-05-04)

Code-reviewer + architect-reviewer parallel run. **6 patches applied** (4 P0 + 2 P1). **1 P0 surfaced to Steve**: live admin password committed to `CLAUDE.md`.

## P0 patches APPLIED (local — NOT deployed)

| # | File | Issue | Fix |
|---|---|---|---|
| 1 | `src/routes/inquiries.ts:86,112` | `GET /api/inquiries` + `GET /api/inquiries/:id` were unauthenticated (only the site-wide HTTP Basic stood between them and any visitor). Returned all guest **PII**: name, email, phone, message of every inquiry | Added `requireAuth` to both GET routes |
| 2 | `src/routes/activities.ts` (5 routes) | `POST /activities`, `PUT /activities/:id`, `DELETE /activities/:id`, `POST /activities/:id/reviews/refresh`, `POST /activities/bulk-import` had **zero auth**. Anyone with site basic-auth could create/destroy activities or bulk-import arbitrary CSV/JSON | Added `requireAuth` to all 5 mutating routes |
| 3 | `src/routes/linkValidation.ts` | `POST /api/link-validation/check-url` was an unauthenticated SSRF — POST `{"url":"http://169.254.169.254/..."}` and the server fetched it. No allowlist. | `router.use(requireAuth)` at top — entire link-validation router admin-only |
| 4 | `src/services/linkValidationService.ts:18` | `checkLink()` had no scheme/IP allowlist. Even with auth gate above, defense-in-depth: stale admin session shouldn't be able to probe internal infra | Reject non-http(s) protocols + RFC1918 (10/8, 172.16/12, 192.168/16) + loopback (127/8, ::1) + link-local (169.254/16) + CGNAT (100.64/10) |

## P1 patches APPLIED

| # | File | Issue | Fix |
|---|---|---|---|
| 5 | `src/middleware/auth.ts:21` | Malformed `Authorization` header (e.g. `Bearer` no space) crashed with `Buffer.from(undefined,'base64')` and leaked 500 stack trace | Validate `parts.length === 2 && parts[0] === 'Basic'` before decoding |
| 6 | `src/middleware/auth.ts:25` | Password compared with plain `===` — leaks length via timing | `timingSafeEqual` from `node:crypto` after length-equal check |

`tsc` not installed in node_modules; typecheck skipped. All edits are minimal additions (no signature changes).

## P0 NEEDS STEVE

**`CLAUDE.md` lines 6-7 contain live admin credentials in plaintext** (`Username: admin / Password: DWSecure2024!`). The file is not in `.gitignore`. If the repo ever gets pushed or shared, the credentials are exposed.

Required actions:
1. Remove the credential block from `CLAUDE.md` (replace with "see /secrets skill for current admin password")
2. Add `CLAUDE.md` to `.gitignore` (or at minimum stop committing credentials to it)
3. **Rotate `DWSecure2024!`** — also used by George email agent + several other DW services per MEMORY. Mint via `/secrets` skill, fan to all consumers.
4. Check git history: `cd ~/Projects/jill-website && git log --all -- CLAUDE.md` to see how long it's been there.

## P1 / P2 deferred (in REVIEW)

- No `helmet` / security headers (npm install needed).
- Session middleware configured (`server.ts:68-77`) but **never used** — `req.session.adminUser` is declared on line 41-46 but never assigned. Either ship session-auth and rip basic-auth, or rip out sessions. Carrying both is confusing.
- No CSRF protection on any POST route (sameSite cookie absent on session config).
- No rate limit on `POST /api/inquiries` — email-bomb vector (each retry = 3 SMTP attempts × 15s).
- `bulk-import` multer has no `fileFilter` — accepts any MIME up to 5MB before the post-upload type check runs.
- Unsubscribe (`unsubscribe.ts:21`) accepts any `?email=` with no HMAC token — anyone can unsubscribe anyone (currently a no-op in DB but planned).
- `src/migrations/002_create_cms_tables.sql:50` seeds `admin_users` with bcrypt of `admin123` — committed default password.
- `linkValidationService` follows 5 redirects to any destination — final URL after redirect chain not re-checked against private-IP block.
- `parseInt` on linkValidation query params lacks bounds check (high `limit` returns unbounded result).

## Architecture roadmap

**Architectural Impact: MEDIUM.** Codebase in good shape (proper MVC, factory routes, singleton pool, real Jest tests). NOT a directory-vertical, NOT a factory, NOT a drops-engine — bespoke 4-property vacation rental site with inquiry CRM + CMS bolted on.

### Shared-lib verdict: **NOT a directory-core consumer**

| Lib | Fit | Reason |
|---|---|---|
| `directory-core` | NO | 4-unit catalog, not a directory of N entities |
| `factory-core` | NO | No pipeline/stages/critic loop |
| `drops-core` | NO | No newsletter engine |
| `place-graph-core` | NO | No address-history concept |
| **`safe-fetch` micro-lib** | **YES** (future) | `linkValidationService` here + factory-core's URL-fetcher + any future stayclaim crawler all need the same RFC1918 block. The patch I just applied to `checkLink` is a candidate for extraction once a 2nd consumer materializes. |
| **Google Maps/Reviews service** | MAYBE | `googleMapsService.ts` + `googleReviewsService.ts` (~530 LOC) reusable verbatim if stayclaim's place-detail pages ship. YAGNI for now. |

### Top 5 highest-leverage structural changes (from architect)

1. **Delete `server.js` + `test-server.js` at repo root** — pre-TS-rewrite leftovers (PM2 runs `dist/server.js`). Confuses every onboarding agent.
2. **Extract `src/routes/pages.ts`** — array-driven `[{path:'/beaches', view:'beaches'}, ...]` replacing `server.ts:135-222`. The 4 hardcoded property routes (`/properties/luna|sol|casita|main-house`) should be data-driven from the same `properties` table that powers `/property/:id`. Adding a 5th property today requires a code change + redeploy.
3. **Pick one auth model.** Either keep both basic-auths and **delete express-session + the SessionData declaration** (currently configured but never assigned), or move admin to session-based login and rip basic-auth out. Right now you're paying complexity of both with benefits of neither.
4. **Add `private hydrate(row): Property` to PropertyModel** (and PageContent/Activity equivalents). The JSONB serialize/deserialize logic is duplicated 4× in `Property.ts` (lines 134-140, 160-166, 181-187, 235-241). Reduces Property.ts from 261 → ~180 LOC.
5. **Harden `linkValidationService.checkLink` further** — the patch I added blocks at hostname level. Full DNS-resolve-then-recheck (resolve A records, reject if any falls in private ranges) would defeat DNS rebinding. Extract to shared `safe-fetch` lib once factory-core ships.

## Files touched

- `/Users/stevestudio2/Projects/jill-website/src/middleware/auth.ts`
- `/Users/stevestudio2/Projects/jill-website/src/routes/inquiries.ts`
- `/Users/stevestudio2/Projects/jill-website/src/routes/activities.ts`
- `/Users/stevestudio2/Projects/jill-website/src/routes/linkValidation.ts`
- `/Users/stevestudio2/Projects/jill-website/src/services/linkValidationService.ts`