arnaudne/Epicure
1
0
Fork 0
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

fix: close SSRF/rebinding, IDOR, and stale-session authz gaps found in audit

Browse Source 
Bump to 0.5.1. Fixes: unfollowed-redirect SSRF + DNS-rebinding in AI
url-import and webhook dispatch (new safeFetch with IP-pinned undici
dispatcher); cross-user photo deletion via unvalidated recipe/review
storage keys; comment-moderation and tier-quota checks trusting a
stale cached session role/tier instead of the DB.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Arnaud
2026-07-12 19:05:20 +02:00
parent 4f36ce24b2
commit 5a9e306357
17 changed files with 307 additions and 54 deletions
Download Patch File Download Diff File Expand all files Collapse all files
SECURITY_AUDIT.md
+97
View File
@@ -0,0 +1,97 @@
# Security Audit — Epicure
Date: 2026-07-12
Scope: full app (`apps/web`, `packages/db`), 3 independent rounds, each own lens.
Bar: confidence ≥8/10 only.
---
## Round 1 — Auth & Authorization
### Finding 1: Stale cached role trusted for comment moderation — MEDIUM
- File: `apps/web/app/api/v1/comments/[id]/route.ts:33`
- Confidence: 8
- Description: Route checks `session.user.role === "user"` directly. Role comes from Better Auth `cookieCache`, stale up to 5 min (`apps/web/lib/auth/server.ts:79-82`). `requireAdmin()` (`apps/web/lib/api-auth.ts:17-30`) already works around this by re-querying DB — this route doesn't.
- Exploit: Admin demotes abusive moderator/admin to `user`. Demoted user's existing session still passes stale-role check for up to 5 min → can keep deleting other users' comments via `DELETE /api/v1/comments/[id]`.
- Fix: Reuse fresh-DB-role pattern from `requireAdmin()` (or shared `requireRole()` helper) instead of trusting `session.user.role` directly.
### Finding 2: Tier entitlement not re-verified against DB — LOW/MEDIUM
- Files: `apps/web/lib/auth/server.ts:79-82` (cookieCache) + all quota call sites (`api/v1/ai/generate/route.ts`, `ai/adapt/[id]/route.ts`, `ai/meal-plan/generate/route.ts`, `ai/batch-cook/generate/route.ts`, `apps/web/lib/tiers.ts:31`)
- Confidence: 8
- Description: Same cookieCache staleness as Finding 1 applies to `session.user.tier`, but `checkAndIncrementTierLimit` never re-checks DB tier.
- Exploit: User downgraded pro→free (cancellation/chargeback). Stale session keeps pro-tier quota (AI calls, recipe count, storage) applying for up to 5 min. Bounded quota-bypass, not data breach.
- Fix: Fetch `users.tier` fresh in `checkAndIncrementTierLimit`, mirroring `requireAdmin`.
**No other findings ≥8** — recipes/comments/ratings/meal-plans/shopping-lists/collections/admin routes/api-keys/webhooks/Better Auth config/proxy.ts/public share pages all reviewed clean.
---
## Round 2 — Injection & Input Validation
### Finding 1: SSRF via unfollowed redirect in AI URL-import — HIGH
- File: `apps/web/lib/ai/features/import-url.ts:22-28`
- Confidence: 9
- Description: `POST /api/v1/ai/import-url` validates host via `validateWebhookUrl()` (blocks private/loopback/metadata ranges) but then `fetch(url, {...})` uses default `redirect: "follow"` — no restriction on where a 3xx response redirects to. `apps/web/lib/webhooks.ts:39-49` already hardens this with `redirect: "manual"` + comment explaining why; `import-url.ts` never got the same fix.
- Exploit: Attacker hosts public page returning `302 Location: http://169.254.169.254/latest/meta-data/...` (or internal service). Initial host passes validation; fetch follows redirect straight to internal/metadata address, response fed into AI prompt.
- Fix: Set `redirect: "manual"` in `import-url.ts`, treat 3xx as failure (mirror `webhooks.ts`).
### Finding 2: DNS-rebinding TOCTOU in `validateWebhookUrl` — MEDIUM
- Files: `apps/web/lib/validate-webhook-url.ts:96-123`, consumed by `import-url.ts:22-25`
- Confidence: 8
- Description: Validation does its own `dns.lookup`; the real `fetch()` re-resolves independently moments later. Attacker controlling authoritative DNS (low TTL) can return public IP for validation, private IP for fetch. `webhooks.ts` has a comment accepting this as residual risk; `import-url.ts` has no such mitigation and also lacks redirect protection (compounds with Finding 1).
- Fix: Resolve host once, validate resolved IP, connect directly to that IP (pin via custom `dns.lookup`/agent) instead of re-resolving in `fetch`. Apply to both `webhooks.ts` and `import-url.ts`.
### Finding 3: IDOR into S3 `deleteObject` via unvalidated photo storage keys — HIGH
- Files: `apps/web/app/api/v1/recipes/route.ts:48-51`, `apps/web/app/api/v1/recipes/[id]/route.ts:45-48,211-231`, sink `apps/web/lib/storage.ts:40-42`
- Confidence: 8
- Description: `photos[].key` validated only as non-empty string — no check that key was actually issued to this user/recipe by `/api/v1/upload/presign` (no prefix check against `recipes/{recipeId}/photos/` + `session.user.id`). Storage keys of public recipes are visible in rendered `<Image>` src on `apps/web/app/r/[id]/page.tsx:52-58`. On `PUT`, any photo key present in old list but absent from new list gets passed to `deleteObject()` with no ownership check.
- Exploit: Attacker views victim's public recipe, copies photo storage key from image URL. Calls `PUT /api/v1/recipes/{ownRecipeId}` with `photos: [{key: <victim's key>}]` (accepted, no ownership check), then `PUT` again without that key → diff logic treats it as "removed" → `deleteObject` permanently deletes victim's S3 object. No privilege required.
- Fix: Track storage keys server-side as belonging to a specific recipe/user (row created at presign time, or HMAC-signed key). Validate every submitted photo key was issued for this recipe/user before accepting into `recipePhotos` or before deletion eligibility. At minimum prefix-match against `recipes/{recipeId}/` + `session.user.id`, and never `deleteObject` a key not already present in that recipe's existing photo set.
**No other findings ≥8** — all `sql\`...\`` usages parameterized correctly, no eval/exec usage, no raw filesystem path construction from user filenames elsewhere, Zod schemas otherwise bound ranges/enums/lengths properly.
---
## Round 3 — Crypto, Secrets Management & Data Exposure
**No findings ≥8.** Reviewed: `lib/auth/server.ts`, `lib/encrypt.ts`, `lib/gravatar.ts`, `lib/invites.ts`, `lib/api-auth.ts`, shopping-list public-share feature (`app/s/[id]/page.tsx`, `api/v1/shopping-lists/**`), profile-picture upload, `users/me/export`, api-keys/ai-keys, webhooks, Stripe/cron signature verification, broad sweep of routes joining/returning `users` rows.
Notable good patterns confirmed:
- Share-link tokens use `crypto.randomUUID()` (122 bits), scoped queries never leak owner PII.
- User-row queries always project to narrow DTO before serializing (password/email never leak).
- All security-sensitive tokens use `crypto.randomBytes`/`randomUUID`; `Math.random()` only in non-security contexts.
- `lib/encrypt.ts`: AES-256-GCM + HKDF-SHA256, random IV per encryption, auth-tag verified.
- Webhook/cron secret checks use `crypto.timingSafeEqual` with length pre-check.
- No TLS-verification bypasses, no hardcoded secrets.
- `requireAdmin()` re-queries DB role rather than trusting session cache (see Round 1 Finding 1 — inconsistently applied elsewhere).
---
## Summary
| # | Severity | Category | Location | Confidence |
|---|----------|----------|----------|------------|
| R2-F1 | HIGH | SSRF (redirect) | `lib/ai/features/import-url.ts:22-28` | 9 |
| R2-F3 | HIGH | IDOR / arbitrary delete | `api/v1/recipes/[id]/route.ts:211-231` | 8 |
| R1-F1 | MEDIUM | Stale-role authz bypass | `api/v1/comments/[id]/route.ts:33` | 8 |
| R2-F2 | MEDIUM | SSRF (DNS rebinding) | `lib/validate-webhook-url.ts:96-123` | 8 |
| R1-F2 | LOW/MEDIUM | Stale-tier quota bypass | `lib/tiers.ts:31` + call sites | 8 |
**Priority fix order**: R2-F1 (SSRF redirect) and R2-F3 (IDOR delete) first — both HIGH, both concrete exploit paths with no special privilege needed. Then R1-F1 (moderation bypass), R2-F2 (rebinding, compounds with F1), R1-F2 (quota bypass, low impact).
**Status: all 5 fixed** (`safeFetch` w/ IP-pinning + manual-redirect in `lib/validate-webhook-url.ts`; `isOwnedRecipePhotoKey` in `lib/storage.ts` enforced on recipe create/update; fresh-DB role check in `comments/[id]/route.ts`; fresh-DB tier check in `lib/tiers.ts`).
---
## Round 4 — Verification + fresh sweep
Re-verified all 5 fixes above: all PASS, no gaps, no regressions (full `pnpm typecheck` + `pnpm test` clean, same 2 pre-existing unrelated test failures as baseline `main`).
### Finding: IDOR on rating/review `photoKey` — same class as R2-F3, different endpoint — HIGH (fixed)
- File: `apps/web/app/api/v1/recipes/[id]/rate/route.ts:10,41,54`
- Confidence: 9
- Description: `POST /api/v1/recipes/[id]/rate` accepted a client-supplied `photoKey` with no ownership check — the R2-F3 fix only covered `recipePhotos` (folder `photos/`), not review photos (folder `reviews/`), which use a different prefix format and weren't validated at all.
- Exploit: Attacker reads a victim's review-photo storage key off any public recipe/review response, submits it as `photoKey` when rating an unrelated recipe. When that recipe's author later deletes their recipe, `recipes/[id]/route.ts` DELETE collects all `ratings.photoKey` for the recipe and calls `deleteObject()` unconditionally — destroying the victim's photo in a completely different recipe.
- Fix applied: added `isOwnedReviewPhotoKey(key, recipeId, userId)` to `lib/storage.ts` (checks `recipes/{recipeId}/reviews/{userId}-` prefix), enforced in `rate/route.ts` before insert/update — rejects with 400 if the submitted key wasn't issued to this user for this recipe's reviews.
No other new HIGH/MEDIUM findings survived this round. Checked clean: admin role checks, internal cron auth, public shopping-list share endpoints, all raw `sql` usage, other `deleteObject`/presign call sites.