stupid-simple-network-inventory/SECURITY_DECISIONS.md

# Security Decisions

Decisions, trade-offs, and documented limitations for Stupid Simple Network Inventory.

---

## JWT in localStorage vs HttpOnly cookie

**Decision**: keep JWT in `localStorage` + Bearer token.

**Rationale**: migrating to HttpOnly cookies requires changes to the Nginx config (cookie proxying, SameSite handling), the FastAPI auth flow, and all frontend API calls. The added complexity is disproportionate for a self-hosted LAN tool. The risk is mitigated by:
- 24-hour token expiry (reduced from 7 days)
- `token_version` invalidation: password change immediately revokes all prior tokens
- The application is designed for trusted LAN use, not public internet exposure

**Assumption**: the deployment is behind a trusted proxy or on a private network where XSS is the primary concern and is partially mitigated by the LAN context.

**Future**: if the app ever needs public internet exposure, migrating to `HttpOnly; Secure; SameSite=Strict` cookies should be the first priority.

---

## Token versioning instead of a session store

**Decision**: `token_version` integer column on `User`, incremented on password change.

**Rationale**: no Redis, no external session store. A single integer per user provides immediate token invalidation with zero dependencies. The version is included in the JWT payload (`ver` field) and validated on every request.

**Trade-off**: revocation is per-user, not per-token. Logging out one device still lets other devices use their tokens until the common password is changed. This is acceptable for a single-user or small-team tool.

**Limitation**: `ver` absent from old tokens is treated as `ver=1` for backward compatibility. Existing valid tokens (before `token_version` was added) will therefore continue to work until the user changes their password.

---

## no-new-privileges absent from backend container

**Decision**: omit `security_opt: no-new-privileges:true` for the backend service.

**Rationale**: the discovery feature uses `subprocess.run(["ping", ...])`. In `iputils-ping` on Debian, the ping binary has the file capability `cap_net_raw=ep`. For a non-root process to execute ping successfully, the `execve` call must be allowed to promote capabilities from the file's permitted set. `no-new-privileges` blocks this promotion, breaking the ping feature.

**Alternatives considered**:
- Ambient capabilities (Linux 4.3+): would require modifying the entrypoint to `prctl(PR_CAP_AMBIENT_RAISE, ...)` before exec. Adds complexity with no meaningful security gain for this threat model.
- SUID ping: `no-new-privileges` also blocks SUID.
- Replace subprocess ping with a Python ICMP implementation: significant refactor out of scope.

**Mitigations in place**: `cap_drop: ALL` + `cap_add: NET_RAW DAC_OVERRIDE` limits the container to only the capabilities needed. `DAC_OVERRIDE` is required because the `db_data/` bind-mount is owned by the host user; root without it cannot create SQLite journal files in that directory. Both capabilities are in Docker's default set, so this is a net reduction from a standard container.

---

## CORS default to `*`

**Decision**: `ALLOWED_ORIGINS` defaults to `"*"` for backward compatibility.

**Rationale**: the application is designed for same-origin access via Nginx (browser → Nginx → FastAPI), so CORS headers are not required for normal operation. However, changing the default to empty (no CORS) would silently break integrations where users access the API from a different origin. The default `"*"` maintains existing behavior while making it configurable.

**Production recommendation**: set `ALLOWED_ORIGINS=` (empty) if the backend is only accessed through the Nginx proxy, or set it to the specific domain if cross-origin access is needed.

---

## In-memory rate limiting

**Decision**: Python `dict[str, list[float]]` protected by `threading.Lock`, no Redis.

**Rationale**: Redis adds operational complexity (another service, volume, potential failure mode). For a single-process Uvicorn deployment behind Nginx, in-memory rate limiting is sufficient. The Nginx `limit_req` module provides the primary protection at the network edge; the Python layer adds defense-in-depth.

**Limitations**:
- State is lost on container restart (acceptable: a restart already breaks all active sessions).
- Does not work across multiple workers. Uvicorn defaults to a single worker; if scaled, the Nginx layer remains effective.
- IP tracking sees the Nginx container's IP (Docker internal), not the real client IP, unless `X-Real-IP` is forwarded to the backend. The Nginx `limit_req` uses the real client IP correctly.

---

## APP_UID / APP_GID build args

**Decision**: configurable container UID/GID via build args, defaulting to 1000.

**Rationale**: the backend bind-mounts `./db_data:/app/data`. For the non-root container user to write to this directory, the container UID must match the host directory owner. On most Linux systems, the first non-root user is UID 1000. The build args allow customisation without Dockerfile changes.

**Alternative**: use an entrypoint that `chown`s the directory at runtime (current implementation via `entrypoint.sh`). This ensures compatibility regardless of UID mismatch, at the cost of a brief root execution before dropping to `appuser`.

---

## nginx-unprivileged for frontend container

**Decision**: use `nginxinc/nginx-unprivileged:alpine` as the frontend base image.

**Rationale**: the standard `nginx:alpine` image requires `CAP_CHOWN` and `CAP_SETUID` at startup — the master process (root) chowns temp directories and forks workers as `nginx` user (UID 101). With `cap_drop: ALL`, both syscalls fail. Two alternative approaches were tried and abandoned:
- `USER nginx` in the Dockerfile: BusyBox `sed` does not support `\s` in basic regex mode, so the PID path substitution silently failed; the entrypoint scripts also need root.
- `gosu appuser` in the backend: `CAP_SETUID` is dropped, so `gosu` cannot switch UIDs.

`nginxinc/nginx-unprivileged:alpine` is maintained by nginx Inc. and pre-configures nginx to run entirely as UID 101 without needing `CAP_CHOWN` or `CAP_SETUID`. The entire process tree (master + workers) runs as nginx, making `cap_drop: ALL` compatible with zero extra capabilities on the frontend.

**Trade-off**: adds a dependency on a third-party image (`nginxinc/nginx-unprivileged`) instead of the official `nginx` image. The image is maintained by nginx Inc. itself, so the trust model is equivalent.

---

## ping and NET_RAW capability

**Decision**: retain `cap_add: NET_RAW` on the backend container.

**Rationale**: the discovery feature (`/api/discovery/ping`, `/api/discovery/scan`) uses ICMP ping, which requires `CAP_NET_RAW`. This is the minimum capability needed and is explicitly documented. The capability is dropped from the frontend container which has no need for it.

---

## SQLite without foreign key enforcement

**Status**: known limitation, not addressed in this phase.

SQLite does not enforce foreign keys by default. Enabling `PRAGMA foreign_keys=ON` per connection via a SQLAlchemy event was out of scope for this phase. The application code manually handles cascades (explicit DELETE before device removal). This is documented in SEC-FIX-013 as a future fix.

---

## Future security improvements (not scheduled)

| ID | Topic |
|----|-------|
| SEC-FIX-006 | Scope discovery scan to inventory CIDRs, validate IPs |
| SEC-FIX-007 | Pydantic field constraints on business models |
| SEC-FIX-008 | HTTP security headers (CSP, X-Content-Type-Options, etc.) |
| SEC-FIX-009 | Document TLS / reverse-proxy contract more explicitly |
| SEC-FIX-012 | Structured audit logging |
| SEC-FIX-013 | SQLite foreign key enforcement |
| SEC-FIX-014 | `npm audit` / `pip-audit` in CI |
| SEC-FIX-015 | Import JSON schema validation and size limits |
| SEC-FIX-016 | Remove `v-html` from App.vue |