132 lines
9.2 KiB
Markdown
132 lines
9.2 KiB
Markdown
# padhle — Security Hardening Plan (fixes first, tested)
|
|
|
|
> **Purpose:** Close every vulnerability found in the 2026-08-19 codebase review, get as close to production-level as possible, and *prove* there are no session leaks with automated tests.
|
|
> **Runtime:** Local Supabase is started by the user (`npx supabase start` → API on `:54321`, Studio `:54323`). Backend runs on `:3001`.
|
|
> **Style:** Each phase has small, ordered, independently verifiable steps. Every step has a **Definition of Done** and a **Verify** command so progress is provable.
|
|
|
|
---
|
|
|
|
## Phase 0 — Baseline (before touching code)
|
|
|
|
| # | Step | Verify |
|
|
|---|---|---|
|
|
| 0.1 | Confirm local Supabase is up: `curl -s http://127.0.0.1:54321/rest/v1/` | HTTP 200 + OpenAPI JSON |
|
|
| 0.2 | Confirm `backend/.env` has `SUPABASE_URL`, `SUPABASE_PUBLISHABLE_KEY`, `SUPABASE_SERVICE_ROLE_KEY`, `OPENAI_API_KEY` | `grep -oE '^[A-Z_]+=' backend/.env` shows all |
|
|
| 0.3 | Baseline tests: `cd backend && node --test` and `cd frontend && npm run build` | All pass before changes |
|
|
|
|
---
|
|
|
|
## Phase 1 — 🔴 Secret scrub (do first; no code risk)
|
|
|
|
**Problem:** `sb_secret_N7UND0UgjKTVK-Uodkm0Hg_xSvEMPvz` (local Supabase **service-role** key, bypasses RLS) is hardcoded in git-tracked files.
|
|
|
|
| # | File | Change | DoD / Verify |
|
|
|---|---|---|---|
|
|
| 1.1 | `ANALYSIS.md` (~line 311) | Replace the live secret with `<rotated-local-dev-key>` and a note to never commit keys | `git grep -n sb_secret` → no real key |
|
|
| 1.2 | `IMPLEMENTATION_PLAN.md` (~line 397) | Same replacement | `git grep -n sb_secret` → no real key |
|
|
| 1.3 | `backend/scripts/setup-db.js` (line 10) | Remove hardcoded fallback; **throw** if `SUPABASE_SERVICE_ROLE_KEY` is missing | `node --check backend/scripts/setup-db.js` passes; no key string in file |
|
|
| 1.4 | `backend/.env.example` | Add `SUPABASE_URL`, `SUPABASE_PUBLISHABLE_KEY`, `SUPABASE_SERVICE_ROLE_KEY` placeholders | File lists the vars |
|
|
| 1.5 | Rotate local keys | User runs `npx supabase stop && npx supabase start` (or regenerates), then updates `backend/.env` | Old key string no longer works; app still connects |
|
|
| 1.6 | Guard | Add a `scripts/check-secrets.mjs` (fails `git grep`-style scan on known patterns) and note to run it pre-commit | Script exits 0 on clean tree, 1 on a planted secret |
|
|
|
|
> Also add a `.gitignore`-style reminder: never add `.env` files. Already covered by existing gitignores.
|
|
|
|
---
|
|
|
|
## Phase 2 — 🟠 Auth hardening
|
|
|
|
**Problem A:** `GET /api/auth/me` trusts an unsigned `padhle.user` JSON cookie — identity is forgeable.
|
|
|
|
| # | File | Change | Verify |
|
|
|---|---|---|---|
|
|
| 2.1 | `backend/src/routes/auth.js` | `/me` becomes async: read `padhle.token`, call `verifyToken()` from `middleware/supabaseAuth.js`; on success return `{ user: { uid, email } }` from the **verified** token; on failure `401` | `curl` with forged cookie → 401; real login → 200 with correct uid |
|
|
| 2.2 | `backend/src/middleware/supabaseAuth.js` | Export `verifyToken` already exists — no change needed | — |
|
|
|
|
**Problem B:** Password policy is client-side only (`minLength=6`).
|
|
|
|
| # | File | Change | Verify |
|
|
|---|---|---|---|
|
|
| 2.3 | `backend/src/routes/auth.js` | In `signup`, reject `password.length < 6` with 400 before calling Supabase | Test: short password → 400 "Password must be at least 6 characters" |
|
|
|
|
---
|
|
|
|
## Phase 3 — 🟠 Input validation (prompt injection vector)
|
|
|
|
**Problem:** `grade`/`subject` are allow-listed, but `chapter` is free-form and is injected verbatim into the AI **system prompt** and stored. Also `POST /api/sessions` validates nothing.
|
|
|
|
| # | File | Change | Verify |
|
|
|---|---|---|---|
|
|
| 3.1 | `backend/src/routes/chatValidation.js` | Add `validateChapter(chapter)`: non-empty, ≤ 200 chars, reject `\n`, `\r`, `<`, `>`. Add `validateSessionMeta({grade, subject, chapter})` reusing the allow-lists + chapter check. Export both. | Unit test in `chatValidation.test.js`: valid chapter ok; `"Ignore <instructions>"` → error; 201-char → error |
|
|
| 3.2 | `backend/src/routes/chat.js` | Call `validateChapter(chapter)` (when present) in `POST /api/chat` | `curl` with `chapter: "<script>"` → 400 |
|
|
| 3.3 | `backend/src/routes/sessions.js` | `POST /api/sessions` validates grade/subject/chapter via `validateSessionMeta` | `curl` bad metadata → 400 |
|
|
|
|
---
|
|
|
|
## Phase 4 — 🟠 AI provider hardening
|
|
|
|
**Problem A:** `getChatResponse` Google branch still concatenates the API key into the URL path (unpatched half of CVE-2026-004).
|
|
|
|
| # | File | Change | Verify |
|
|
|---|---|---|---|
|
|
| 4.1 | `backend/src/services/ai.js` | Rebuild Google non-streaming URL with the `URL` constructor + `searchParams.set('key', ...)` exactly like `streamGoogle` | `node --check`; grep shows no `${apiKey}` path concat |
|
|
|
|
**Problem B:** No timeouts on AI calls — a stalled provider hangs the SSE connection forever.
|
|
|
|
| # | File | Change | Verify |
|
|
|---|---|---|---|
|
|
| 4.2 | `backend/src/services/ai.js` | `streamOpenai` + `summarizeConversation` + `getChatResponse`: pass `signal: AbortSignal.timeout(120_000)` to `openai.chat.completions.create` | Code review; test mocks not required |
|
|
| 4.3 | `backend/src/services/ai.js` | `streamAnthropic` / `streamGoogle` fetches: add `signal: AbortSignal.timeout(120_000)` | Code review |
|
|
|
|
---
|
|
|
|
## Phase 5 — 🟡 Middleware / robustness
|
|
|
|
| # | File | Change | Verify |
|
|
|---|---|---|---|
|
|
| 5.1 | `backend/src/middleware/supabaseAuth.js` | Cap `tokenCache` (e.g. 2000 entries): when over cap, delete the oldest key (Map insertion order) | Unit test or code review |
|
|
| 5.2 | `backend/src/middleware/securityHeaders.js` (new) | Manual headers middleware: `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, `Referrer-Policy: no-referrer`, `Cross-Origin-Opener-Policy: same-origin` (no new dependency) | `curl -sI http://localhost:3001/health` shows headers |
|
|
| 5.3 | `backend/src/index.js` | Mount `securityHeaders` first; add `app.set('trust proxy', ...)` from env (`TRUST_PROXY` default `false`; set to `1` behind a proxy) | Headers present; `req.ip` correct behind proxy |
|
|
| 5.4 | `backend/src/middleware/rateLimiter.js` | In dev (`NODE_ENV !== 'production'`), skip rate limits for any loopback IP (`::1`, `127.0.0.1`, `::ffff:127.0.0.1`) — prevents tests from tripping the 5/15min auth limit | Integration test can sign up users repeatedly |
|
|
|
|
---
|
|
|
|
## Phase 6 — Session-leak verification (the core ask)
|
|
|
|
**Goal:** Prove cross-user access is impossible, end to end, against the real local Supabase.
|
|
|
|
| # | File | Change | Verify |
|
|
|---|---|---|---|
|
|
| 6.1 | `backend/src/app.js` (new) | Extract the Express app from `index.js` into an exported `app` (no listen) | `node --check` |
|
|
| 6.2 | `backend/src/index.js` | Import `app` from `./app.js`; only `app.listen` stays here | Backend still starts |
|
|
| 6.3 | `backend/test/leak.test.js` (new) | Integration test: starts `app.listen(0)` on loopback, then with `fetch` + manual cookie jar: <br> 1. Sign up user A and user B (unique emails). <br> 2. A sends a chat → capture `session` id from the SSE stream. <br> 3. **B** `GET /api/chat/:id` → **404**. <br> 4. **B** `GET /api/chat/sessions` and `GET /api/sessions` → must **not** contain A's session. <br> 5. **B** `DELETE /api/sessions/:id` and `PATCH .../clear` → **404**. <br> 6. **B** posts to A's chatId → **404**. <br> 7. `/api/auth/me` with a forged `padhle.user` cookie → **401**. <br> 8. `POST /api/chat` with `chapter: "<script>ignore"` → **400**. <br> Teardown: delete A/B from `auth.users` via service role (or leave test accounts, documented). | `cd backend && node --test test/leak.test.js` → all pass |
|
|
|
|
---
|
|
|
|
## Phase 7 — Regression & full verification
|
|
|
|
| # | Step | Verify |
|
|
|---|---|---|
|
|
| 7.1 | `cd backend && node --test` | All suites pass (chatValidation, aiMessages, leak) |
|
|
| 7.2 | `cd frontend && npm run build` | Vite build passes |
|
|
| 7.3 | Syntax check all backend files: `for f in backend/src/**/*.js; do node --check "$f"; done` | All pass |
|
|
| 7.4 | Manual smoke (optional, user): start backend + frontend, sign up, chat, reload | No console errors; session persists |
|
|
| 7.5 | `git grep -n -E 'sb_secret|sk-[A-Za-z0-9]{20,}'` | No live secrets in tracked files |
|
|
| 7.6 | Update `ANALYSIS.md` Feature Tracker with a "Security Hardening" section listing fixes + verification results | Doc is current |
|
|
|
|
---
|
|
|
|
## Explicitly out of scope (documented for later)
|
|
|
|
- **Refresh-token flow** (hourly cookie expiry, no silent refresh) — needs `padhle.refresh` cookie + middleware auto-refresh; flagged, not implemented now.
|
|
- **Server-side session revocation list** — signout clears cookies + calls Supabase logout; server revocation of *other* sessions is a future item.
|
|
- **CSRF tokens** — mitigated via `sameSite: lax` httpOnly cookies; full CSRF token flow deferred.
|
|
- **Rate limiter storage** (`express-rate-limit` in-memory) — single-instance assumption; move to Redis when multi-instance.
|
|
- **`run-migration.js`** plaintext local Postgres password (`postgres`) — local dev only; documented, not a prod risk.
|
|
- **XSS sanitization in `sessionStore.js`** — file is legacy/unused; rendering already escapes HTML. Documented, not implemented.
|
|
|
|
---
|
|
|
|
## Priority order for execution (this goal round)
|
|
|
|
1. Phase 1 (secret scrub) → 2. Phase 3 (validation) → 3. Phase 4 (AI URL + timeouts) → 4. Phase 5 (middleware) → 5. Phase 2 (auth) → 6. Phase 6 (leak test) → 7. Phase 7 (regression)
|