# Security & Bug Audit — Roosterwijs

Date: 2026-06-09 · Scope: backend (Django), frontend (React), Docker/nginx deployment.
Verdict: code quality is good and the architecture is clean, but the system is **not safe to expose on a network yet**. One critical gap (no authentication) and two high-risk items must be fixed before the test server holds real student data.

> **STATUS 2026-06-10: all findings below are FIXED** (verified with 13 functional tests + frontend build). Remaining for you: (1) run `npm install` once in `frontend/` and commit `package-lock.json` so Docker switches to `npm ci`; (2) set `DJANGO_SECURE=1` in `.env` once the server runs behind HTTPS. Note #2 (TLS itself) still requires a certificate/reverse proxy on the server.

---

## CRITICAL

### 1. The entire API is unauthenticated
`REST_FRAMEWORK` has no `DEFAULT_PERMISSION_CLASSES`, so DRF defaults to `AllowAny`. Anyone who can reach port 8080 can, anonymously:

- read, create, edit and **delete all persons** — including student names and the free-text `opmerkingen` field (sensitive special-education data → AVG/GDPR);
- delete groups and subgroups;
- enable/disable modules via `POST /api/modules/<key>/state/`.

**Fix:**

```python
# settings.py
REST_FRAMEWORK = {
    "DEFAULT_PERMISSION_CLASSES": ["rest_framework.permissions.IsAuthenticated"],
    "DEFAULT_AUTHENTICATION_CLASSES": [
        "rest_framework.authentication.SessionAuthentication",
    ],
    ...
}
```

Then add a login flow to the React app and send the CSRF token (see Low #11 — these must land together, otherwise all writes break). This naturally belongs to the `accounts` module, but a minimal session login must protect the API **before** that module is fully built — security cannot be an optional plugin.

---

## HIGH

### 2. No TLS anywhere; admin login travels in plain text
`docker-compose.yml` exposes plain HTTP on 8080 and `.env.example` shows `http://` CSRF origins. Django admin passwords and all student data cross the network unencrypted.

**Fix:** terminate TLS (reverse proxy or certs in nginx), then enable in `settings.py`, driven by an env flag:

```python
if _env_bool("DJANGO_SECURE", not DEBUG):
    SECURE_PROXY_SSL_HEADER = ("HTTP_X_FORWARDED_PROTO", "https")
    SECURE_SSL_REDIRECT = True
    SESSION_COOKIE_SECURE = True
    CSRF_COOKIE_SECURE = True
    SECURE_HSTS_SECONDS = 31536000
```

### 3. SECRET_KEY silently falls back to a known dev key
If `DJANGO_SECRET_KEY` is missing in production, the app runs with `"dev-only-change-me-in-productie"` — sessions and password-reset tokens become forgeable, with no warning.

**Fix:** fail hard:

```python
SECRET_KEY = os.environ.get("DJANGO_SECRET_KEY", "")
if not SECRET_KEY:
    if _env_bool("DJANGO_DEBUG", False):
        SECRET_KEY = "dev-only-change-me-in-productie"
    else:
        raise RuntimeError("DJANGO_SECRET_KEY ontbreekt; weiger te starten zonder geheime sleutel.")
```

(Note: `DEBUG` is read after this block today — reorder accordingly.)

---

## MEDIUM

### 4. Browsable API enabled in production
`BrowsableAPIRenderer` gives visitors a friendly UI to explore and submit writes. Gate it: include it in `DEFAULT_RENDERER_CLASSES` only when `DEBUG` is true.

### 5. Bug — module toggle truthiness
`plugins/views.py`: `enabled = bool(request.data.get("enabled", False))`. A form-encoded or string payload `"false"` becomes `True`. Parse explicitly:

```python
enabled = request.data.get("enabled")
if not isinstance(enabled, bool):
    return Response({"detail": "enabled moet true of false zijn."}, status=400)
```

### 6. Bug — role change leaves stale subgroup membership
`Persoon.save()` clears `groep` when the rol is no longer *leerling*, but M2M `subgroepen` membership stays (`limit_choices_to` only affects admin forms, not the API). A teacher can remain "lid" of a niveaugroepje.

**Fix:** in `Persoon.save()` (after `super().save()`): `if self.rol != Rol.LEERLING: self.subgroepen.clear()` — or validate in `PersoonSerializer.update()`.

### 7. No rate limiting
No DRF throttling and no nginx `limit_req`. Add `DEFAULT_THROTTLE_CLASSES`/`RATES` (e.g. `"anon": "20/min"`, `"user": "200/min"`) — cheap insurance once the API is internet-reachable.

### 8. Backend container runs as root
Add to the Dockerfile after `COPY . .`:

```dockerfile
RUN useradd --create-home appuser && chown -R appuser /app
USER appuser
```

(Static volume permissions: run `collectstatic` before switching user, or chown the volume in entrypoint.)

---

## LOW

### 9. Race in `sync_states()`
Three gunicorn workers can pass the `key not in existing` check simultaneously → `IntegrityError` on the unique key. Use `ModuleState.objects.get_or_create(key=spec.key, defaults={"enabled": spec.default_enabled})`.

### 10. Missing security headers on the SPA
Django adds `X-Frame-Options`/`nosniff` only to `/api` and `/admin`. Add to the nginx `location /` block: `X-Content-Type-Options: nosniff`, `X-Frame-Options: DENY`, `Referrer-Policy: same-origin`, and later a CSP.

### 11. Frontend sends no CSRF token
`api.js` never sends `X-CSRFToken`. Harmless today (anonymous = no CSRF check), but the moment Fix #1 lands, every POST/PATCH/DELETE will 403. Read the `csrftoken` cookie and set the header in `request()`. Ship together with #1.

### 12. `npm install` in Docker build
Use `npm ci` with a committed `package-lock.json` for reproducible, tamper-evident builds.

### 13. No logging / audit trail
No `LOGGING` config; with student data you want a record of logins and destructive actions. Minimal console logging config now; an audit-log module fits the plugin architecture later.

---

## Verified non-issues

- `.gitignore` and `backend/.dockerignore` correctly exclude `.env`, `db.sqlite3` (image and repo stay clean).
- CORS is locked to localhost dev origins; production traffic is same-origin via nginx — correct.
- `ALLOWED_HOSTS` from env with safe defaults; nginx forwards `Host` so host-header spoofing is caught.
- Dependency-aware module enable/disable logic in `plugins/services.py` is sound (checked both directions).
- Postgres is not port-mapped to the host; only nginx is exposed.
- Strong password validators active; `DEBUG` defaults to off.

## Recommended order of work

1. #1 + #11 together (auth + CSRF in frontend) — blocks everything else being meaningful.
2. #3 (fail hard on missing SECRET_KEY) — one-file change, do immediately.
3. #2 (TLS + secure cookies) when the test server gets a hostname.
4. #5, #6, #9 (bugs) — small, independent fixes.
5. #4, #7, #8, #10, #12, #13 as hardening follow-ups.