nas-tools/docs/improvement-plan.md

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.