# NAS Tools — Holistic Improvement Plan (PDD) > 37 prioritized issues from 4 deep-dive audits (CI/config, codebase structure, dashboard security, code quality) + the original 21-item plan. Duplicates consolidated, ranked by true priority. > > **Sprint specs:** `specs/sprint-*.md` — each with checkboxes, ACs, and exit criteria. --- ## Priority 0 — Security: Immediate Hardening (this week) These are exploitable vulnerabilities, not architectural concerns. Fix them first. ### P0.1 — OPC WebSocket has zero authentication - **File:** `dashboard/backend/routers/opc_ws.py:66-84` - **Problem:** `/ws/opc` accepts any connection without token, cookie, or credential of any kind. Once connected, clients receive real-time broadcasts of task updates, agent execution results, agent status changes, and internal data (`broadcast_task_update`, `broadcast_agent_execution`, `broadcast_agent_status`). - **Fix:** Require a valid JWT token via query parameter or cookie on WebSocket connect (FastAPI/Starlette supports `Depends` on WebSocket endpoints). Reject unauthenticated connections with 403. - **Also:** Add per-user connection tracking so broadcasts scope to authorized users only (`opc_ws.py:16` flat set of all connections). ### P0.2 — Hardcoded Transmission credentials (`admin/admin`) - **File:** `dashboard/backend/routers/transmission.py:17,41` - **Problem:** `auth=("admin", "admin")` hardcoded in source. Anyone with network access to Transmission port 9091 can use these known credentials. - **Fix:** Read credentials from env vars (`TRANSMISSION_USER`/`TRANSMISSION_PASS`) with no default. Require them at startup. ### P0.3 — Remove hardcoded fallback SECRET_KEY from dev compose - **File:** `dashboard/docker-compose.dev.yml:21` - **Problem:** `SECRET_KEY=${SECRET_KEY:-c0e8dcd7...}` — if env var is unset, a known hex string becomes the live JWT signing key. Attackers who obtain this can forge any access/refresh token. - **Fix:** Remove the default value. Make `SECRET_KEY` mandatory (fail fast with clear error if unset). ### P0.4 — Passkey register/options endpoint has no rate limiting - **File:** `dashboard/backend/routers/passkey.py:58-72` - **Problem:** `passkey_register_options` has no `@limiter.limit` decorator. An attacker can flood the endpoint generating unlimited WebAuthn challenges, consuming server memory (`_challenges` dict) and CPU. - **Fix:** Add `@limiter.limit("5/minute")` or similar rate limit. --- ## Priority 1 — Production Stability (this week) ### P1.1 — deploy-dev.yml deploys without running tests - **File:** `.gitea/workflows/deploy-dev.yml` - **Problem:** The workflow has no test job and no `needs` dependency on `test.yml`. Every push to `dev` bypasses all tests and deploys directly. This contradicts the documented behavior in `IMPROVEMENTS.md` (line 69-74), which claims tests gate dev deploys. - **Fix:** Either inline the test jobs into `deploy-dev.yml` with `needs`, or make `deploy-dev.yml` depend on a separate test workflow (if Gitea supports cross-workflow dependencies). At minimum, add the same backend/frontend test jobs that `deploy.yml` has. ### P1.2 — Dev deployment broken (runner mismatch) - **File:** `.gitea/workflows/deploy-dev.yml:15` - **Problem:** Uses `runs-on: ubuntu-latest` but tries to access NAS paths (`/volume1/docker/`). Must be `runs-on: nas`. - **Fix:** Change runner label to `nas`. ### P1.3 — Production dashboard missing docker-socket-proxy - **File:** `dashboard/docker-compose.yml` - **Problem:** Compose defines docker-socket-proxy service but it may not be running, causing Docker monitoring timeouts. - **Fix:** Ensure the full compose stack (including docker-socket-proxy) is deployed. Verify with `docker compose ps`. ### P1.4 — Docker.svelte has no error handling - **File:** `dashboard/frontend/src/routes/Docker.svelte:12-15` - **Problem:** `containers = (await get("/docker/containers")) || []` — if the Docker API is unreachable, the thrown exception crashes the component with no UI feedback. - **Fix:** Wrap in try/catch, show error state in UI, provide retry button. ### P1.5 — `psutil.cpu_percent(interval=0)` always returns 0 on first call - **File:** `dashboard/backend/routers/system.py:53` - **Problem:** `cpu_percent(interval=0)` uses a cached value with no sampling, so the first call after server start reports 0%. This is a data accuracy bug visible to users. - **Fix:** Use `interval=0.1` or call `cpu_percent()` once at startup to prime the counter. --- ## Priority 2 — Auth & Access Control (next 2 weeks) ### P2.1 — RBAC endpoints use raw JSON (no Pydantic models) - **Files:** `dashboard/backend/routers/auth.py:242-259` (`rbac_set_override`), `:303-326` (`rbac_update_role`) - **Problem:** Both endpoints use `await request.json()` directly. Only one field (`pages`) is validated; any other keys silently pass through to the data store. - **Fix:** Define Pydantic models (`RbacOverrideRequest`, `RbacUpdateRoleRequest`) with explicit fields and validation. ### P2.2 — Passkey endpoints use raw JSON (no Pydantic) - **File:** `dashboard/backend/routers/passkey.py:78,127,188` - **Problem:** `register_verify`, `login_verify`, `delete` all parse `await request.json()` ad-hoc. - **Fix:** Define Pydantic models matching the WebAuthn payloads. ### P2.3 — Passkey challenge store is a global dict (no cleanup, race-prone) - **File:** `dashboard/backend/routers/passkey.py:29-55` - **Problem:** `_challenges` is an in-memory global dict. TTL cleanup exists but challenges are popped on use — a race between legitimate user and attacker consuming the same challenge. - **Fix:** Bind challenges to session tokens so only the session that requested the challenge can consume it. ### P2.4 — COOKIE_SECURE=False on auth cookies - **File:** `dashboard/backend/auth_service.py:23` - **Problem:** JWT cookies are not marked `Secure`. The comment says this is intentional (Caddy terminates TLS), but if Caddy config changes or backend is ever exposed directly, tokens leak over HTTP. - **Fix:** Keep as-is for now (it works with Caddy), but add a startup check: if `COOKIE_SECURE=False`, log a prominent warning and gate it behind an explicit env var (`ALLOW_INSECURE_COOKIES=true`). ### P2.5 — Audit log endpoint exposes IPs and usernames - **File:** `dashboard/backend/routers/system.py:82-116` - **Problem:** `/api/system/audit-log` serves client IPs and usernames to any user with "dashboard" page access. This is a privacy leak. - **Fix:** Gate behind admin role, or redact IPs/usernames for non-admin viewers. ### P2.6 — Security log exposes Authelia failed-login usernames - **File:** `dashboard/backend/routers/security.py:55-98` - **Problem:** `_parse_authelia_logs` extracts usernames and IPs from failed login attempts and serves them via `/api/security/logs`. Any user with "security" page access can see who is trying (and failing) to log in. - **Fix:** Gate behind admin role. Consider redacting usernames, showing only counts. ### P2.7 — LoginRequest model has no max_length constraints - **File:** `dashboard/backend/routers/auth.py:22-25` - **Problem:** Username and password fields have no `max_length`. While pbkdf2_sha256 bounds overhead, maliciously long strings can still cause resource exhaustion upstream (request parsing, logging). - **Fix:** Add `max_length=128` for username, `max_length=1024` for password. ### P2.8 — Fragile `opc_db.json.dumps()` pattern - **File:** `dashboard/backend/services/agent_executor.py:273,307` - **Problem:** Uses `opc_db.json.dumps(result)` — relies on `opc_db.py` importing `json` at module level. If that import is refactored or replaced with `orjson`, these calls break silently. - **Fix:** Import `json` directly in `agent_executor.py` instead of reaching through `opc_db`. --- ## Priority 3 — Configuration & Hardening (next 2-3 weeks) ### P3.1 — Centralize hardcoded IPs and URLs - **Frontend:** `Sidebar.svelte:30-31` (LAN_IP, TS_IP), `:42-58` (subdomain URLs for Navidrome, Jellyfin, Immich, Gitea, n8n, etc.) - **Backend:** `config.py:25-39` (SSH host IPs, usernames, key paths), `email_service.py:68,81,110,142` (hardcoded `nas.jimmygan.com`) - **Fix:** Move all URLs/IPs to environment variables or a single config endpoint. For frontend, add a `/api/config/external-services` endpoint that returns the service URLs so they can be changed without rebuilding the frontend. ### P3.2 — Root `.gitignore` is too thin - **File:** `.gitignore` (11 lines) - **Problem:** Missing `venv/`, `.DS_Store`, `*.pyc`, `.vscode/`, `*.log`, `.env.local`, `.env.production`. Only covers `node_modules/`, `dist/`, `.env`, `*.tar.gz`, `__pycache__/`. - **Fix:** Add common patterns from `dashboard/backend/.gitignore` at root level: `.DS_Store`, `*.pyc`, `venv/`, `.vscode/`, `.idea/`, `*.log`. ### P3.3 — Add secret scanning to CI - **Problem:** No automated check for accidentally committed secrets. `.env` files with real passwords exist on disk (e.g., `immich/.env` has `DB_PASSWORD=immich_nas_2026`). A git slip could leak credentials. - **Fix:** Add a `gitleaks detect` step to the `test.yml` workflow (runs on PRs to main). Low false-positive rate if configured correctly. ### P3.4 — Missing `.env.example` for Immich - **File:** `immich/` (has `.env` with real password, no `.env.example`) - **Fix:** Create `immich/.env.example` with placeholder values, matching the pattern used by `openclaw/`, `watchtower/`, `dashboard/`, etc. ### P3.5 — CSP allows `wss:` and `ws:` globally - **File:** `dashboard/backend/main.py:148` - **Problem:** `connect-src 'self' wss: ws:` allows WebSocket connections to any origin. Should restrict to `'self'` only. - **Fix:** Change to `connect-src 'self'` (WebSocket upgrades from same origin are covered by `'self'`). ### P3.6 — Standalone Limiter instances in router modules - **Files:** `routers/auth.py:19`, `routers/passkey.py:25` - **Problem:** Both create separate `Limiter(key_func=get_remote_address)` instances outside the app context. The `slowapi` library expects the limiter to be attached to `app.state.limiter` (done in `main.py:92`). These standalone instances may not integrate correctly. - **Fix:** Use `from main import app` and reference `app.state.limiter`, or use `request.app.state.limiter` in endpoint functions. Alternatively, verify these standalone limiters work correctly with the current slowapi version and document the pattern. --- ## Priority 4 — CI/CD & Build Reliability (next 3-4 weeks) ### P4.1 — DOCKER_BUILDKIT=0 everywhere without documentation - **Files:** `deploy.yml:211`, `deploy-dev.yml:53`, `deploy-claude-dev-dev.yml:56,65` - **Problem:** BuildKit is disabled globally but the reason (Synology ContainerManager compatibility) is not documented in the workflows or CLAUDE.md. This sacrifices build caching and performance. - **Fix:** Add a comment in each workflow explaining why `DOCKER_BUILDKIT=0` is needed. Re-test with BuildKit enabled on the current DSM version — ContainerManager may support it now. ### P4.2 — `--cache-from` without `--cache-to` (cache never persisted) - **Files:** `deploy.yml:211`, `deploy-dev.yml:53`, `deploy-claude-dev-dev.yml:67` - **Problem:** `--cache-from nas-dashboard:latest` reads cache layers from the image, but without `--cache-to`, new cache layers are never written back. Consecutive builds on the same runner benefit from Docker's local layer cache, but `--cache-from` by named reference is only effective if the image is present locally. - **Fix:** Add `--cache-to type=inline` (embeds cache metadata in the image) or `--cache-to type=registry,ref=...` if a registry is available. ### P4.3 — Node.js/Python versions not pinned in CI - **Files:** `test.yml:26-27,119-120`, `deploy.yml:27-28,103-104` - **Problem:** Uses `python3 --version` and `node --version` which depend on whatever `ubuntu-latest` ships. Non-reproducible builds. - **Fix:** Pin versions explicitly. For Python: use `python:3.12-slim` container or `actions/setup-python`. For Node: use `node:20-alpine` container or `actions/setup-node`. ### P4.4 — Dead `test-summary` job in deploy.yml - **File:** `deploy.yml:173-189` - **Problem:** The `test-summary` job is not a dependency of `deploy` (which depends directly on `backend-tests` and `frontend-tests`), so it runs in parallel with deploy and has no effect. - **Fix:** Either remove it or make `deploy` depend on `test-summary` instead of the individual test jobs. ### P4.5 — required-tools.txt vs Dockerfile.base discrepancy - **Files:** `claude-dev/required-tools.txt`, `claude-dev/Dockerfile.base` - **Problem:** `bash`, `ca-certificates`, and `openssh-client` are installed in the base image but not in `required-tools.txt`. Either the smoke test is incomplete or the image installs unnecessary packages. - **Fix:** Reconcile — either add these to `required-tools.txt` (if they're required at runtime) or remove them from `Dockerfile.base` (if they're build-only dependencies). ### P4.6 — Stale debug comment in Dockerfile - **File:** `claude-dev/Dockerfile:11` — `# CI test 1775295475` - **Fix:** Remove the stale comment. ### P4.7 — Orphaned .md files in `.gitea/workflows/` - **Files:** `TEST_RESULTS.md`, `IMPROVEMENTS.md` - **Problem:** These are not referenced by any workflow, not linked from CLAUDE.md, and contain dated information (2026-04-21). They will rot. - **Fix:** Move key content into CLAUDE.md or `docs/`, then delete the originals. --- ## Priority 5 — Operations & Resilience (next month) ### P5.1 — Missing HEALTHCHECK in claude-dev Docker image - **Files:** `claude-dev/Dockerfile`, `claude-dev/docker-compose.yml` - **Problem:** No HEALTHCHECK instruction in Dockerfile and no `healthcheck` block in compose. CI post-deploy uses ad-hoc `docker exec` commands. - **Fix:** Add `HEALTHCHECK --interval=30s --timeout=5s CMD claude --version || exit 1` to Dockerfile, and/or add `healthcheck` to compose. ### P5.2 — Missing resource constraints on production containers - **Files:** `claude-dev/docker-compose.yml`, `dashboard/docker-compose.yml` - **Problem:** Dev compose has CPU/memory limits; production doesn't. A memory leak or CPU spike can impact other services on the same host. - **Fix:** Add `mem_limit`, `cpus`, and `restart_policy` to production compose files. Start with generous limits, tighten based on observed usage. ### P5.3 — 40MB audit log with no rotation - **File:** `/volume1/docker/nas-dashboard/audit.log` (~40MB and growing) - **Problem:** No log rotation, no retention policy. Will eventually fill the disk. - **Fix:** Implement `logging.handlers.RotatingFileHandler` in `main.py` audit middleware (max 10MB, keep 5 backups). Consider structured logging to SQLite for queryability. ### P5.4 — Immich ML model download failures - **Problem:** Phone app cannot upload photos because ML models (`buffalo_l`, `ViT-B-32__openai`) fail to download from `modelscope.cn` and other sources. Cache directory issues prevent retry. - **Fix:** 1. Pre-download models to `/volume1/docker/immich/model-cache` manually 2. Set `MACHINE_LEARNING_REQUEST_TIMEOUT` to increase download timeout 3. Add `IMMICH_MACHINE_LEARNING_ENABLED=false` as temporary fallback to restore uploads without ML 4. Consider configuring a model download mirror for better connectivity ### P5.5 — No backup strategy for dashboard data - **Data at risk:** `opc.db`, `auth.json`, `rbac.json`, audit log - **Fix:** Add backup job to the existing `backup.sh` script. Document restore procedure. ### P5.6 — No monitoring or alerting - **Problem:** No visibility into service health beyond manual log checks. - **Fix (minimal):** Add a `/health` endpoint to dashboard backend that checks DB connectivity, Docker socket, and disk space. Wire it to a simple cron-based alert (Telegram notification on failure). - **Fix (aspirational):** Prometheus metrics endpoint + Grafana dashboard on the NAS. --- ## Priority 6 — Code Quality & Refactoring (next 2 months) ### P6.1 — Frontend route organization - **Problem:** 21 route files in flat `dashboard/frontend/src/routes/` directory. - **Fix:** Group into subdirectories: `routes/media/` (Navidrome, Jellyfin, etc.), `routes/tools/` (Gitea, Transmission, etc.), `routes/admin/` (Security, Settings). ### P6.2 — Backend router auto-discovery - **Problem:** 18 routers individually imported in `main.py` with 40+ import lines. - **Fix:** Use a router auto-discovery pattern — iterate `routers/` directory, import modules dynamically, include their routers. ### P6.3 — Container monitor lacks retry/backoff - **Problem:** Production logs show "Read timed out" errors. Container monitor crashes on Docker socket timeout with no retry. - **Fix:** Add exponential backoff for Docker socket connections, circuit breaker pattern, and health check recovery logic. ### P6.4 — Network cleanup - **Problem:** Multiple overlapping Docker networks (`nas-dashboard_dashboard`, `nas-dashboard_dashboard_internal`, `nas-dashboard_internal`, `internal`, `gitea_gitea`). Some may be unused. - **Fix:** Audit and remove unused networks, standardize naming, document topology. ### P6.5 — CI workflow consolidation - **Problem:** Three similar deploy workflows with subtle differences (deploy.yml, deploy-dev.yml, deploy-claude-dev-dev.yml). - **Fix:** Extract shared steps into reusable composite actions or workflow templates (if Gitea Actions supports them). At minimum, standardize runner labels and build patterns. ### P6.6 — `window.isSecureContext` in Login.svelte at module scope - **File:** `dashboard/frontend/src/routes/Login.svelte:11` - **Problem:** `let showPasswordForm = $state(!window.isSecureContext)` runs at module scope. If SSR is ever enabled, `window` is undefined and the component crashes. - **Fix:** Move into `onMount` or guard with `typeof window !== 'undefined'`. ### P6.7 — Legacy refresh token in localStorage - **File:** `dashboard/frontend/src/lib/api.js:27` - **Problem:** Code reads `localStorage.getItem("refresh_token")` with comment "legacy refresh token". If a stale token exists from a previous session, it may be reused. - **Fix:** If the legacy flow is truly deprecated, remove the localStorage read. If it's a fallback, document when it applies and add expiry checks. --- ## Summary: Execution Order | Phase | When | Items | Impact | |-------|------|-------|--------| | **P0** | This week | P0.1–P0.4 (security hardening) | Prevents exploitation | | **P1** | This week | P1.1–P1.5 (production stability) | Restores dev env, fixes broken features | | **P2** | Next 2 weeks | P2.1–P2.8 (auth & access control) | Hardens auth surface | | **P3** | Next 2-3 weeks | P3.1–P3.6 (configuration & hardening) | Reduces attack surface, prevents config drift | | **P4** | Next 3-4 weeks | P4.1–P4.7 (CI/CD reliability) | Faster, more reliable builds | | **P5** | Next month | P5.1–P5.6 (operations & resilience) | Prevents data loss, improves uptime | | **P6** | Next 2 months | P6.1–P6.7 (code quality) | Maintainability, developer velocity | **Total: 37 prioritized items** across 6 phases, from immediate security fixes to long-term refactoring.