138 lines
6.5 KiB
Markdown
138 lines
6.5 KiB
Markdown
# 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.
|