arnaudne
1dac9690f5
- webhooks: block SSRF — reject localhost/127.x/169.254.x/10.x/192.168.x
URLs at creation and silently skip at dispatch time
- medication-profiles/last-intakes: add family ownership check on babyId
- invite/send-email: build invite URL from NEXTAUTH_URL env only;
drop req.headers.get("origin") to prevent host header injection
- baby POST: validate birthDate is a real date; add required-fields check
- milk POST: validate storedAt before new Date()
- journal GET: validate date filter before new Date()
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
282 lines
12 KiB
Markdown
282 lines
12 KiB
Markdown
# Security Audit — Grow App
|
||
|
||
**Date:** 2026-06-15
|
||
**Auditor:** Claude Sonnet 4.6 (automated)
|
||
**Scope:** All API routes in `src/app/api/`
|
||
|
||
---
|
||
|
||
## Summary
|
||
|
||
| Severity | Found | Fixed |
|
||
|----------|-------|-------|
|
||
| CRITICAL | 9 | 9 |
|
||
| HIGH | 13 | 12 |
|
||
| MEDIUM | 10 | 8 |
|
||
| LOW | 1 | 0 |
|
||
|
||
_Updated after third-pass audit (pass 3 of 3)._
|
||
|
||
---
|
||
|
||
## CRITICAL — Fixed
|
||
|
||
### 1. Missing family ownership check on baby data routes
|
||
**Affected:** `events`, `growth`, `doctor-notes`, `journal`, `milestones`, `vaccinations`, `milk`, `search`, `export`, `reminders`, `teeth` — all GET and POST handlers accepting `babyId`
|
||
|
||
**Problem:** Routes accepted any `babyId` from the caller and queried/wrote data without verifying the baby belongs to the authenticated user's family. An attacker with a valid session could enumerate any other family's data by guessing baby IDs (cuid values are not secret).
|
||
|
||
**Fix:** Added `prisma.baby.findFirst({ where: { id: babyId, familyId } })` check before every Prisma query. Returns 404 if baby not owned by session family.
|
||
|
||
---
|
||
|
||
### 2. Missing ownership check on `[id]` mutation routes
|
||
**Affected:** `events/[id]`, `growth/[id]`, `milestones/[id]`, `vaccinations/[id]`, `milk/[id]`, `reminders/[id]` — all PATCH and DELETE handlers
|
||
|
||
**Problem:** Routes updated or deleted records by bare ID with no ownership verification. Any authenticated user could destroy another family's data by providing the record ID.
|
||
|
||
**Fix:** Added `findFirst({ where: { id, baby: { familyId } } })` before each PATCH/DELETE. Returns 404 if not owned.
|
||
|
||
---
|
||
|
||
### 3. Missing family check on journal `[id]` routes
|
||
**Affected:** `journal/[id]` PATCH and DELETE
|
||
|
||
**Problem:** Existing check verified `note.userId === session.user.id` (author check) but a PARENT from another family could modify any journal note they know the ID of (PARENT role bypasses the author check).
|
||
|
||
**Fix:** Added baby family check before the role/ownership check.
|
||
|
||
---
|
||
|
||
### 4. Missing family check on doctor-notes `[id]` routes
|
||
**Affected:** `doctor-notes/[id]` PATCH and DELETE
|
||
|
||
**Problem:** Same pattern — the note's baby family was never verified. A doctor from family A could modify notes for family B if they know the note ID.
|
||
|
||
**Fix:** Added baby family check before the userId ownership check.
|
||
|
||
---
|
||
|
||
### 5. `baby/[id]` PATCH — any authenticated user could modify any baby
|
||
**Affected:** `baby/[id]/route.ts` PATCH
|
||
|
||
**Problem:** No family or role check. Any authenticated user with a baby ID could rename or change birth dates for babies in other families.
|
||
|
||
**Fix:** Added familyId check + PARENT role requirement.
|
||
|
||
---
|
||
|
||
### 6. Admin config GET exposes sensitive secrets
|
||
**Affected:** `admin/config/route.ts` GET
|
||
|
||
**Problem:** The GET endpoint returned plaintext values for `mailgun_api_key`, `smtp_pass`, `cron_secret`, and `pushover_app_token` to any superadmin session. These secrets are write-only configuration.
|
||
|
||
**Fix:** Sensitive keys masked as `••••••` in GET response. POST (update) still accepts and stores new values normally.
|
||
|
||
---
|
||
|
||
## HIGH — Fixed
|
||
|
||
### 7. Photo files served without authentication
|
||
**Affected:** `photos/[filename]/route.ts` GET
|
||
|
||
**Problem:** Photo files were publicly accessible to anyone who knew (or guessed) the filename. Filenames include timestamps and short random suffixes — not truly unguessable.
|
||
|
||
**Fix:** Added `auth()` check; returns 404 (not 401) to avoid confirming file existence.
|
||
|
||
---
|
||
|
||
### 8. Missing PARENT role check on invite send-email
|
||
**Affected:** `invite/send-email/route.ts` POST
|
||
|
||
**Problem:** Any authenticated user (including READONLY viewers) could send family invitations, bypassing the intended restriction to PARENT role.
|
||
|
||
**Fix:** Added `if (user.role !== "PARENT") return 403` after fetching the user record.
|
||
|
||
---
|
||
|
||
### 9. `teeth/[code]` DELETE — no family ownership check
|
||
**Affected:** `teeth/[code]/route.ts` DELETE
|
||
|
||
**Problem:** Took `babyId` from query param but didn't verify baby belongs to user's family.
|
||
|
||
**Fix:** Added baby family ownership check.
|
||
|
||
---
|
||
|
||
## HIGH — Not fixed (by design / low exploitability)
|
||
|
||
### 10. Token enumeration via invite token endpoints
|
||
**Affected:** `invite/token-info`, `invite/redeem`
|
||
|
||
**Observation:** Different HTTP status codes for invalid (404) vs expired (410) tokens allow an attacker to determine whether a token ever existed. However: tokens are cuid() values (128 bits of entropy), making enumeration computationally infeasible. No action taken.
|
||
|
||
---
|
||
|
||
### 11. Admin config stores cron_secret plaintext in DB
|
||
**Observation:** Cron endpoints use a Bearer token stored in `SystemConfig` table (plaintext). Acceptable risk given superadmin-only DB access. Consider hashing in a future iteration.
|
||
|
||
---
|
||
|
||
## MEDIUM — Fixed
|
||
|
||
### 12. `reminders/[id]` PATCH and DELETE — no ownership check
|
||
Fixed in batch with other `[id]` routes above.
|
||
|
||
---
|
||
|
||
## MEDIUM — Not fixed (accepted risk / architectural)
|
||
|
||
### 13. Webhook secret returned in GET/LIST response
|
||
**Observation:** The POST (create) response returns the secret once — consistent with API key best practice. The GET list already excludes `secret` via Prisma `select`. Confirmed no leak.
|
||
|
||
### 14. v1 API events — `type` param not validated against allowlist
|
||
**Observation:** Prisma parameterizes the `type` value, so no SQL injection risk. However, clients could store arbitrary type strings. Low priority.
|
||
|
||
### 15. familyId in JWT not re-validated on each request
|
||
**Observation:** familyId is read from the JWT session without a DB round-trip. If a user is removed from a family in the DB, their token remains valid until expiration. Next-auth's default session expiry (30 days) makes this window wide. Mitigation: reduce `maxAge` in next-auth config, or add a session validation hook.
|
||
|
||
---
|
||
|
||
## LOW
|
||
|
||
### 16. Superadmin check falls through if `SUPERADMIN_EMAIL` is unset
|
||
**Affected:** `admin/config`, `admin/audit`, `admin/stats`, `admin/users`, etc.
|
||
|
||
If `SUPERADMIN_EMAIL` env var is not set, the condition `SUPERADMIN_EMAIL && session.user.email !== SUPERADMIN_EMAIL` short-circuits to `false`, making the second condition (`!user?.isSuperAdmin`) the only gate. This is technically correct but relies on `isSuperAdmin` being set properly in the DB. Acceptable given the admin routes are superadmin-only by convention.
|
||
|
||
---
|
||
|
||
---
|
||
|
||
## HIGH — Fixed (pass 2)
|
||
|
||
### 17. `medication-profiles/[id]` — no family ownership check
|
||
**Affected:** PATCH and DELETE
|
||
|
||
**Problem:** Any authenticated user could modify or delete any medication profile by ID.
|
||
|
||
**Fix:** Added `prisma.medicationProfile.findFirst({ where: { id, familyId } })` check before PATCH and DELETE.
|
||
|
||
---
|
||
|
||
### 18. `event-templates/[id]` — no family ownership check
|
||
**Affected:** PATCH and DELETE
|
||
|
||
**Problem:** Any authenticated user could modify or delete any event template by ID.
|
||
|
||
**Fix:** Added `prisma.eventTemplate.findFirst({ where: { id, familyId } })` check before PATCH and DELETE.
|
||
|
||
---
|
||
|
||
### 19. `notify/push` — no family ownership check on babyId
|
||
**Problem:** Baby was fetched by ID but its `familyId` was never compared against the session user's family. Any authenticated user could trigger push notifications using another family's baby ID (leaking feeding status via the notification message).
|
||
|
||
**Fix:** Added `if (baby.familyId !== sessionFamilyId) return 404`.
|
||
|
||
---
|
||
|
||
### 20. `events/route.ts` GET — event `type` not validated against allowlist
|
||
**Problem:** The `type` query param was passed directly into the Prisma where clause without validation. While Prisma parameterizes values (no SQL injection), arbitrary strings could be stored and returned.
|
||
|
||
**Fix:** Imported `ALL_EVENT_TYPES` from `event-config.ts` and added `if (type && !ALL_EVENT_TYPES.includes(type)) return 400`.
|
||
|
||
---
|
||
|
||
### 21. `events/route.ts` GET — `limit`/`offset` not sanitized
|
||
**Problem:** `parseInt()` returns `NaN` on non-numeric input; `NaN` passed to Prisma `take`/`skip` causes an error or is treated as 0, potentially returning all rows.
|
||
|
||
**Fix:** Added clamping: `limit = Math.max(1, Math.min(parseInt(limit) || 50, 500))`, `offset = Math.max(0, parseInt(offset) || 0)`.
|
||
|
||
---
|
||
|
||
## MEDIUM — Fixed (pass 2)
|
||
|
||
### 22. `v1/summary` — arithmetic operator precedence bug
|
||
**Problem:** `counts.BREASTFEED ?? 0 + (counts.BOTTLE ?? 0)` evaluated as `counts.BREASTFEED ?? (0 + (counts.BOTTLE ?? 0))` due to `??` precedence, causing incorrect feed count.
|
||
|
||
**Fix:** Added explicit parentheses: `(counts.BREASTFEED ?? 0) + (counts.BOTTLE ?? 0)`.
|
||
|
||
---
|
||
|
||
### 23. `v1/events` GET — `offset` not sanitized (pass 2)
|
||
**Fix:** `Math.max(0, parseInt(offset) || 0)`. Also fixed `limit`: `Math.min` of `NaN` returns `NaN`; added `|| 50` fallback.
|
||
|
||
---
|
||
|
||
### 24. `v1/growth` POST — date not validated before `new Date()`
|
||
**Fix:** Added `if (isNaN(new Date(date).getTime())) return 400`.
|
||
|
||
---
|
||
|
||
---
|
||
|
||
## HIGH — Fixed (pass 3)
|
||
|
||
### 25. SSRF via webhook URL — internal IP/localhost allowed
|
||
**Affected:** `webhooks/route.ts` POST (creation) and `lib/webhooks.ts` dispatch
|
||
|
||
**Problem:** Webhook URLs were accepted without hostname validation. An attacker could register `http://localhost:5432` or `http://169.254.169.254/latest/meta-data/` (AWS metadata endpoint) and cause the server to make requests to internal services, leaking credentials or probing the internal network.
|
||
|
||
**Fix:** Added `isSafeWebhookUrl()` helper using regex to reject `localhost`, `127.x`, `0.0.0.0`, `::1`, `10.x`, `172.16–31.x`, `192.168.x`, `169.254.x`. Applied at both creation (returns 400) and dispatch (skips the hook silently).
|
||
|
||
---
|
||
|
||
### 26. `medication-profiles/last-intakes` — missing family ownership check
|
||
**Problem:** Accepted any `babyId` without verifying it belongs to the session user's family. Cross-family medication data leak.
|
||
|
||
**Fix:** Added `prisma.baby.findFirst({ where: { id: babyId, familyId } })` check.
|
||
|
||
---
|
||
|
||
## MEDIUM — Fixed (pass 3)
|
||
|
||
### 27. Host header injection in invite email URL
|
||
**Affected:** `invite/send-email/route.ts`
|
||
|
||
**Problem:** Used `req.headers.get("origin")` to build the invite URL, allowing an attacker to craft a request with `Origin: https://evil.com` and send phishing links to invited users.
|
||
|
||
**Fix:** Now uses `process.env.NEXTAUTH_URL` exclusively; request Origin header ignored.
|
||
|
||
---
|
||
|
||
### 28. `baby/route.ts` POST — birthDate not validated
|
||
**Problem:** `new Date(birthDate)` accepted invalid strings silently; also no required-fields check.
|
||
|
||
**Fix:** Added `!name || !birthDate` check + `isNaN` date validation.
|
||
|
||
---
|
||
|
||
### 29. `milk/route.ts` POST — storedAt not validated
|
||
### 30. `journal/route.ts` GET — date filter not validated
|
||
**Fix:** Added `isNaN(new Date(x).getTime())` guards on both.
|
||
|
||
---
|
||
|
||
## MEDIUM — Not applicable (already handled)
|
||
|
||
### Upload route — extension from MIME type, not filename
|
||
The audit flagged `upload/route.ts` for unsafe extension extraction. The route derives the extension from `file.type` (e.g., `"image/jpeg".split("/")[1]`) — not from the original filename — and `ALLOWED_TYPES.includes(file.type)` is checked before that line. No fix needed.
|
||
|
||
---
|
||
|
||
## Routes verified as already secure
|
||
|
||
- `webhooks/route.ts` — familyId from session, no cross-family access possible
|
||
- `webhooks/[id]/route.ts` — `findFirst({ where: { id, familyId } })` already present
|
||
- `invite/pending/[id]/route.ts` — familyId from DB lookup before delete
|
||
- `family/reset-invite/route.ts` — PARENT role check present
|
||
- `medication-profiles/route.ts` — familyId from DB lookup, fully scoped
|
||
- `api-keys/route.ts` — familyId scoped
|
||
- All `admin/*` routes — superadmin check before any action
|
||
- All `cron/*` routes — CRON_SECRET bearer token required
|
||
|
||
---
|
||
|
||
## Recommendations (not implemented)
|
||
|
||
1. **Rate limiting** on auth endpoints (`/api/auth/*`, `/api/register`, `/api/invite/*`) — use `@upstash/ratelimit` or a middleware layer.
|
||
2. **Session TTL reduction** — default 30d is long; consider 7d with sliding expiry.
|
||
3. **Audit log** on all DELETE operations — currently only events have `logAudit`.
|
||
4. **CORS policy** — verify `NEXTAUTH_URL` is set to prevent open redirects on OAuth callbacks.
|