padhle - post migration
This commit is contained in:
@@ -0,0 +1,86 @@
|
||||
# Security Audit — 2026-08-20
|
||||
|
||||
Intensive, test-backed attempt to break the application. No assumptions from memory —
|
||||
every claim was verified against live local Supabase (`:54321`) and the running code.
|
||||
|
||||
## Executive summary
|
||||
The app is in good shape on the obvious surfaces: IDOR is enforced (app-code + now RLS),
|
||||
SSE injection is escaped, XSS is prevented (react-markdown escapes HTML, no raw HTML),
|
||||
CSRF is mitigated (SameSite=Lax + fixed CORS origin), and production rate limits bind.
|
||||
The audit found **3 real vulnerabilities** (all fixed and regression-tested), plus several
|
||||
confirmed non-issues and one accepted by-design limitation.
|
||||
|
||||
---
|
||||
|
||||
## Findings
|
||||
|
||||
### 1. HIGH — RLS silently bypassed for all user-data queries (FIXED)
|
||||
**What:** `backend/src/services/db.js` ran *every* query through a `service_role`
|
||||
superuser client (BYPASSRLS). RLS was correctly enabled with per-user policies, but the
|
||||
app never went through a user-scoped client, so PostgREST RLS never enforced anything.
|
||||
Only hand-written app-code ownership checks (`isOwnedSession`, `session.user_id === userId`)
|
||||
kept users isolated. Any future route that forgot a check would have been a full cross-user
|
||||
data breach with zero DB backstop.
|
||||
**Proof (test `adversarial.test.js`, before fix):** a user-scoped client as B is denied A's
|
||||
row by RLS (0 rows); the backend's `getSession()` as service_role reads it.
|
||||
**Fix:** thread the verified user JWT (`req.token`, set in `supabaseAuth`/`optionalAuth`)
|
||||
into every DB call on signed-in paths (`sessions.js`, `chat.js`). RLS is now the enforced,
|
||||
independent backstop *and* the app-code checks remain as defense-in-depth.
|
||||
**Verified:** `getSession(aId, bToken)` → `null` (RLS blocks B); `getSession(aId, aToken)` →
|
||||
owner row; B still gets 404 through the API.
|
||||
|
||||
### 2. MEDIUM-HIGH — revoked tokens kept authenticating via the token cache (FIXED)
|
||||
**What:** `verifyToken` caches `token → user` for up to 5 minutes but `tokenCache` was never
|
||||
purged on signout. `POST /api/auth/signout` revoked the Supabase session but a captured
|
||||
token still resolved to the user via the cache for the remaining TTL.
|
||||
**Proof (test `adversarial.test.js`, before fix):** after signout, replaying the old
|
||||
`padhle.token` cookie → `/api/auth/me` returned **HTTP 200** (cache hit).
|
||||
**Fix:** added `invalidateToken(token)` in `supabaseAuth.js` and call it from the signout
|
||||
route before revoking server-side.
|
||||
**Verified:** after signout, stale token → **HTTP 401**.
|
||||
|
||||
### 3. MEDIUM — unhandled crash + HTML stack-trace leak on non-string input (FIXED)
|
||||
**What:** `validateChatInput` called `text.trim()` without a `typeof` check. Sending
|
||||
`POST /api/chat` with `{"text":123}` threw synchronously *outside* the route's try/catch,
|
||||
so Express's default handler returned **HTTP 500 with a full HTML stack trace**
|
||||
(`TypeError: text.trim is not a function`, absolute file paths, function names, line
|
||||
numbers) — an information-disclosure / robustness bug.
|
||||
**Proof (test `inputHygiene.test.js`, before fix):** `{"text":123}` → 500 + stack dump.
|
||||
**Fix:**
|
||||
- `chatValidation.js`: type-guard `typeof text !== "string"` → clean 400.
|
||||
- `app.js`: added a global JSON error handler so any future uncaught error returns a generic
|
||||
JSON `Internal server error` and never a stack dump.
|
||||
**Verified:** all non-string forms (`123`, `[]`, `{}`, `true`, arrays) → 400, no stack leak.
|
||||
|
||||
### 4. LOW — anonymous 5-message cap is bypassable by id rotation (ACCEPTED, by design)
|
||||
**What:** the trial budget resets on an unknown/omitted `chatId` (that's the documented
|
||||
"refresh loses the conversation" rule). A determined anonymous client can simply rotate the
|
||||
`chatId` to keep sending past 5 messages.
|
||||
**Why accepted:** explicitly a UX funnel, not a security boundary (documented in
|
||||
`ANALYSIS.md`). Bounded by `chatLimiter` (30/min/IP in production — verified: request 31+
|
||||
returns 429), the in-memory LRU/TTL store (max 500 trials, 30-min reaper), and no DB writes
|
||||
for anonymous traffic. No cross-user data is reachable by rotating.
|
||||
|
||||
---
|
||||
|
||||
## Confirmed non-issues (tested / reasoned, not changed)
|
||||
- **IDOR**: A cannot read/list/delete/clear/continue B's sessions (leak.test.js) — and now
|
||||
RLS is a second layer.
|
||||
- **SSE stream injection**: `sseSafe` escapes newlines / U+2028 / U+2029; JSON.stringify
|
||||
already escapes CR/LF. Asserted by tests.
|
||||
- **XSS**: no `dangerouslySetInnerHTML`, no `rehype-raw`; `react-markdown` escapes raw HTML.
|
||||
- **CSRF**: cookies are `SameSite=Lax` (cross-site POST/fetch won't send them) + CORS locked
|
||||
to `http://localhost:5173`. No state-changing GET endpoints.
|
||||
- **Prompt/credential handling**: AI keys never in URL paths (Google uses
|
||||
`URL.searchParams`); `/me` derives identity from the verified JWT, not the unsigned
|
||||
`padhle.user` cookie.
|
||||
- **Auth input robustness**: non-string email/password on signin/signup → generic 400, no crash.
|
||||
|
||||
## Left as-is (minor)
|
||||
- `auth.js` still logs raw Supabase error descriptions via `console.error` (bypasses
|
||||
`logRedact`). Auth error texts are generic and not capability-bearing today; chat errors
|
||||
already route through `logRedact`. A follow-up can unify.
|
||||
|
||||
## Final state
|
||||
`cd backend && node --test` → **38/38 pass** (added `adversarial` and `inputHygiene` suites).
|
||||
Frontend `npm run build` → passes. `scripts/check-secrets.mjs` → clean.
|
||||
Reference in New Issue
Block a user