arnaudne
9e32766046
- medication-profiles/[id]: verify familyId ownership before PATCH/DELETE - event-templates/[id]: verify familyId ownership before PATCH/DELETE - notify/push: verify baby.familyId matches session family before push - events GET: validate type against ALL_EVENT_TYPES allowlist; sanitize limit (1–500) and offset (≥0) to prevent NaN/unbounded queries - v1/summary: fix operator precedence bug in feeds count calculation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
221 lines
9.3 KiB
Markdown
221 lines
9.3 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 | 11 | 10 |
|
|
| MEDIUM | 7 | 3 |
|
|
| LOW | 1 | 0 |
|
|
|
|
_Updated after second-pass audit (pass 2 of 2)._
|
|
|
|
---
|
|
|
|
## 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)`.
|
|
|
|
---
|
|
|
|
## 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.
|