Gan, Jimmy
ddaf3c6cee
- Add JWT token auth to OPC WebSocket (unauthenticated → 403, per-user tracking) - Externalize Transmission RPC credentials to TRANSMISSION_USER/PASS env vars - Remove hardcoded SECRET_KEY fallback from dev compose - Rate-limit passkey register options endpoint at 5/minute - Add PDD (docs/improvement-plan.md) and sprint specs (specs/) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
262 lines
19 KiB
Markdown
262 lines
19 KiB
Markdown
# 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.
|