`REVIEW-2026-05-04.md`

62 lines
# professional-directory (doctor) — overnight debate-team review (2026-05-04)

Code-reviewer + architect-reviewer spawned in parallel. **6 patches applied locally** (3 P0 security + 2 P1 + 1 hardening). 7 deferred items + architecture roadmap captured below.

## P0 / P1 patches APPLIED (local — NOT deployed)

| # | File:line | Severity | Issue | Fix |
|---|---|---|---|---|
| 1 | `agents/api-agent/server.js:392` | **P0** | `/outreach.csv` exported bulk PII (NPI, license, address, phone, emails) with no auth | Added `requireRole('admin')` |
| 2 | `agents/api-agent/server.js:566` | **P0** | `/export.csv` exported full professionals/orgs tables unauth'd (NPI, license, school) | Added `requireRole('admin')` |
| 3 | `agents/api-agent/server.js:159` | **P0** | `/professionals/:npi` did `SELECT p.*` — leaked `opted_out_at`, future internal columns | Enumerated public allow-list (25 columns); excludes `opted_out`, `opted_out_at` |
| 4 | `agents/crawler-agent/server.js:41` | **P0** | `/run/:id` accepted `req.body.env` and merged into spawned child env — unauth'd env injection | Dropped env passthrough + added `Bearer $CRAWLER_RUN_TOKEN` check (503 until token set) |
| 5 | `agents/ingest-agent/server.js:47` | **P0** | Same env-injection bug on `/run/:importer` | Same fix: dropped env merge + `Bearer $INGEST_RUN_TOKEN` |
| 6 | `agents/api-agent/server.js:591` | **P1** | `/opt-out` unauthenticated — anyone could mass-opt-out the entire directory by POSTing IDs | Added `requireRole('admin')` (interim — proper signed-token-emailed-to-practitioner flow needed for self-service opt-out) |
| 7 | `agents/api-agent/server.js:610` | **P1** | 500 error handler returned `err.message` to client (leaks PG table/column/constraint names) | Returns generic `internal_server_error`; full error in server log; `detail` only in non-prod |

`node --check` passes on all 3 modified files.

## Steve action items (4 — surface in email)

1. **Mint `CRAWLER_RUN_TOKEN` + `INGEST_RUN_TOKEN`** via `secrets` skill, route to pm2 env for pd-crawler + pd-ingest. Until set, `/run/:id` returns 503. (Anything currently calling these endpoints — pd-hawk launchd, manual triggers — needs to send `Authorization: Bearer $TOKEN`.)
2. **`/preview/:orgId` page (server.js:829) hardcodes "5★ Average patient rating · 90s Years of trust"** next to real org names. Per your lawyer-directory-compliance memory (Rule 7.x — fabricated stats next to real names), this is a legal risk for the doctor vertical too. Either label as "example copy", remove entirely from the public preview, or move to admin-only pitch view.
3. **Claim verify link uses `req.protocol + req.get('host')`** at `routes/claims.js:104` (open redirect via spoofed Host). Switch to `PUBLIC_BASE_URL` env var pattern that `data_market.js` already uses.
4. **No helmet/CSP** anywhere in api-agent. `npm install helmet` + `app.use(helmet())` before middleware. Touches package.json so deferred for your review.

## P2 deferred (track in REVIEW)

- `auth.js:47` — `deserializeUser` does `SELECT *` from users; if `hashed_password`/`mfa_secret` columns get added, they'll silently appear on `req.user`. Enumerate columns explicitly.
- `server.js:434` — `/organizations/:id/emails` requires login but not admin role; any authed user can pull all scraped outreach emails for an org.
- `routes/claims.js:138` — claim verify breaks on fresh session (link clicked in incognito). Move `GET /:token` outside the auth guard; use the token itself as proof.
- `data_market.js:505` — Stripe error message included in redirect URL (truncated to 160 chars but can still leak card detail / customer ID). Map to generic "payment configuration error".
- `server.js:144,260` — `LIMIT ${limit} OFFSET ${offset}` are string-interpolated. Currently safe because of `Math.min(Number(...), 500)` validation but one refactor away from regression. Use `$N` params.

## Architecture roadmap (architect-reviewer findings)

**Architectural Impact: MEDIUM** — bones are sound (worker/API split, shared DB module, idempotent migrations, public-edge isolation via pd-preview). 5 specific drifts accumulating cost.

### 1. Collapse 4 worker agents → 1 `pd-worker`
`crawler-agent/server.js`, `dedupe-agent/server.js`, `enrich-agent/server.js`, `ingest-agent/server.js` are structurally identical (4 near-duplicate 50-line Express servers each spawning child scripts from a sibling dir). Crawler has *one* crawler, dedupe has *one* job. The 7-agent fan-out is theatre at this scale.

**Action:** merge into `agents/worker-agent/server.js` with `POST /run/:kind/:id` dispatching to `crawlers/`/`enrichers/`/`matchers/`/`importers/` subdirs. Drops 4 pm2 entries → 1, simplifies hawk-local.sh, eliminates 3 ports. Re-split only when a worker actually needs different memory/concurrency.

### 2. Extract `directory-core` shared package
Lawyer-directory has TS `src/server/{admin,auth,billing,community,data_market,leads,...}.ts`. Doctor has near-identical concepts in JS `agents/api-agent/routes/{auth,claims,subscriptions,reviews,threads,messages,admin-pitch}.js` + `data_market.js`. Two implementations, one TS one JS — diverge on every bug fix. Already 4 verticals (doctor/lawyer/animals/restaurant) per MEMORY.

**Action:** extract `~/Projects/directory-core/` npm workspace exporting (a) migration runner, (b) shared route modules (claims, reviews, threads, data_market, Stripe webhook handler, Google OAuth), (c) apply-templates engine. Each vertical = `directory-core` + `verticals/<name>/` config (DB name, specialty list, mockup hooks, scoring). Settle on TS for the core; JS adapters via `.d.ts` shims. **Without this, every compliance fix lands in 1 of 4 codebases.**

### 3. Break up `agents/api-agent/server.js` (2,310 lines)
Already mounts 9 sub-routers but still inlines `/professionals`, `/organizations`, `/search`, `/website-data/:npi`, `/export.csv`, `/opt-out`, `/preview/:id`, `getCounts()`, `renderDashboard()` (300+ lines of inline HTML).

**Action:** move into `routes/professionals.js`, `routes/organizations.js`, `routes/search.js`, `routes/export.js`, `routes/preview.js`, `admin/dashboard.js`. `server.js` ≤ 200 lines. Precondition for #2.

### 4. Smoke-test harness — anything beats zero
Zero tests. The defensive `throw new Error('stripeWebhookHandler not exported')` at `server.js:42` is a runtime assertion compensating for missing import test.

**Action:** `tests/smoke/` with (a) migrate.test.js boots temp PG db + asserts tables, (b) api-routes.test.js boots pd-api against temp + hits every GET, (c) stripe-webhook.test.js POSTs signed fixture + asserts 200. Wire to `npm test` + `hawk-local.sh`. ~200 LOC prevents the "all 3 sites served wrong HTML" class of incident.

### 5. Codify drvouch deploy story
No Kamatera deploy script, no Dockerfile, no nginx config, no doc in `docs/` mentioning drvouch. `ecosystem.config.js` is local-only (loopback binds, hardcoded `/Users/stevestudio2`). MEMORY lists drvouch but nothing in the repo points there.

**Action:** `docs/deploy-kamatera.md` capturing target topology (which agents on Kamatera vs Mac2, nginx routing to drvouch, secrets fan-out, launchd→systemd translation). `deploy/kamatera-ecosystem.config.js` mirrors local but reads `process.env.PORT_*` + `DATABASE_URL`. Until exists, drvouch is a verbal contract.