v1.0

docs/AUDIT_SLAVE.md

@@ -0,0 +1,386 @@
+# Javis Slave & Slave-MCP — Code Audit
+
+**Scope:** [javis-slave](../javis-slave) (satellite app: touch GUI + headless/Raspberry Pi
+mode, ~3.6 k lines) and [javis-slave-mcp](../javis-slave-mcp) (MCP server exposing the
+satellites' controls to the master, ~1 k lines), including `pyproject.toml`,
+`setup-pi.sh`, the systemd unit and the READMEs.
+
+**Method** — same as [AUDIT.md](AUDIT.md): every file read line by line, `ruff
+--select F,E9,B` baseline (8 findings), and each claim reproduced where possible:
+
+* the MCP tool failure was reproduced by calling the tool through FastMCP
+  (`mcp` 1.29.0 in `javis-slave-mcp/.venv`);
+* the unauthenticated control API was reproduced with FastAPI's `TestClient`;
+* the Windows volume bug was reproduced in a scratch venv with the current `pycaw`
+  (20260927, comtypes 1.4.17).
+
+Line numbers refer to the audited revision `0246cbb6`.
+
+---
+
+## 1. Architecture
+
+```mermaid
+flowchart LR
+    subgraph Satellite["javis-slave (tablet / Raspberry Pi)"]
+        MIC[MicStream + resampler] --> WW[openWakeWord]
+        MIC --> REC[record_utterance + VAD]
+        ENG[AudioEngine] --> SPK[play_pcm]
+        CTRL["Control API :8766<br/>/mcp/v1/*"] --> SYS[volume / brightness / power]
+        UI[Touch UI or headless log]
+    end
+    WW --> ENG
+    REC --> ENG
+    ENG -- "POST /api/v1/turn (Bearer key)" --> MASTER[Javis master :8765]
+    ENG -- "GET /api/v1/config (60 s poll)" --> MASTER
+    MCP[javis-slave-mcp<br/>FastMCP stdio] -- HTTP --> CTRL
+    MASTER -- spawns --> MCP
+    CTRL -- "pass-through /api/v1/slaves" --> MASTER
+```
+
+* The satellite owns the wake word and end-pointing; the master does STT → LLM → TTS
+  and returns WAV.
+* The control API is a FastAPI app served by uvicorn inside the same event loop
+  (qasync in GUI mode, `asyncio.run` headless).
+* `javis-slave-mcp` is spawned by the master's MCP host and calls each configured
+  satellite's control API.
+
+---
+
+## 2. Findings summary
+
+| ID | Sev. | Component | Title | Status |
+|---|---|---|---|---|
+| [MCP-S1](#mcp-s1) | Critical | slave-mcp | Every device tool fails: `asyncio.run()` inside the server's event loop | Fixed |
+| [SLV-01](#slv-01) | Critical | slave / security | Control API (power, mic, volume, master pass-through) has no authentication | Fixed |
+| [SLV-02](#slv-02) | High | slave / Windows | Volume control broken with current `pycaw` | Fixed |
+| [SLV-03](#slv-03) | Medium | slave | Turn upload / pairing retried after a read timeout → duplicate turn, burnt code | Fixed |
+| [SLV-04](#slv-04) | Medium | slave | Failed engine start leaks the open microphone (ALSA: busy forever) | Fixed |
+| [SLV-05](#slv-05) | Medium | slave | Playback thread keeps writing while the stream is closed | Fixed |
+| [SLV-06](#slv-06) | Medium | slave / security | Master-supplied wake-word name used as a file path | Fixed |
+| [SLV-07](#slv-07) | Medium | slave | Corrupt config silently replaced → device_id and pairing lost | Fixed |
+| [SLV-08](#slv-08) | Medium | slave / security | `slave_config.json` (API key) world-readable on Linux | Fixed |
+| [MCP-S2](#mcp-s2) | Medium | slave-mcp | `slave_get_ip_address` looks up the alias instead of the device UUID | Fixed |
+| [MCP-S3](#mcp-s3) | Medium | slave-mcp | Power tools not marked destructive | Fixed (annotations) |
+| [SLV-09](#slv-09) | Low | slave / Windows | Power commands report success regardless of exit code | Fixed |
+| [SLV-10](#slv-10) | Low | slave | Retry counter double-counted; misleading "re-pair manually" error | Fixed |
+| [SLV-11](#slv-11) | Low | slave | 30 ms chunks zero-padded to Silero's 512-sample window | Fixed |
+| [SLV-12](#slv-12) | Low | slave | Mic callback can raise during shutdown | Fixed |
+| [SLV-13](#slv-13) | Low | slave | Fire-and-forget tasks without strong references | Fixed |
+| [SLV-14](#slv-14) | Low | slave / GUI | Transcript bubbles render LLM text as rich text | Fixed |
+| [MCP-S4](#mcp-s4) | Low | slave-mcp | `JAVIS_SLAVE_DEVICES` parsing fragile | Fixed |
+| [MCP-S5](#mcp-s5) | Low | slave-mcp | 25 copy-pasted tool bodies, client per call, README drift | Fixed |
+| [LINT-S](#lint-s) | Low | both | Ruff F/B findings | Fixed |
+| [SLV-15](#slv-15) | Info | slave | `windows_control.py` is an unused copy of `system_control.py` | Open (delete?) |
+| [SLV-16](#slv-16) | Info | slave | uvicorn's signal capture overrides the headless SIGTERM handler | Fixed (pin) |
+| [SLV-17](#slv-17) | Info | slave / security | Plain HTTP; the satellite does not authenticate the master | Open (recommendation) |
+| [SLV-18](#slv-18) | Info | Pi deployment | systemd unit has no sandboxing | Open (recommendation) |
+
+---
+
+## 3. Detailed findings
+
+### javis-slave-mcp
+
+#### MCP-S1
+**Every device tool fails: `asyncio.run()` inside the server's event loop** — *Critical*
+
+All 24 device tools are plain `def` functions that call
+`asyncio.run(_slaves[id].get_status())` (server.py L319–L816). FastMCP 1.x calls a
+synchronous tool **directly on its running event loop**
+(`FuncMetadata.call_fn_with_arg_validation`: `return fn(**args)`), and `asyncio.run`
+refuses to start a second loop there. Reproduced:
+
+```
+slave_get_volume → ERROR: asyncio.run() cannot be called from a running event loop
+```
+
+So "mach die Küche leiser" could never work; the master only ever got `ERROR:` back.
+**Fix:** all tools are `async def` and await the client directly; one `_run()` helper
+raises failures as `ToolError`, which FastMCP returns as a result with
+`isError: true` (the master's MCP host turns that into its usual `ERROR:` text).
+
+#### MCP-S2
+**`slave_get_ip_address` looks up the alias instead of the device UUID** — *Medium*
+
+`get_slave_ip(device_id)` passes the MCP alias (`"localhost"`, `"kitchen"`) to
+`/mcp/v1/master/slaves/{id}/ip`, but the master keys satellites by their UUID, so the
+lookup always returned 404. **Fix:** the tool first reads the satellite's real
+`device_id` from `/mcp/v1/status`.
+
+#### MCP-S3
+**Power tools not marked destructive** — *Medium*
+
+`slave_shutdown`, `slave_restart_system`, `slave_sleep` are indistinguishable from
+read-only tools, and the shipped `mcp_servers.json` sets `"approval": "allow"` for this
+server, so a mis-heard request can power off a satellite without confirmation.
+**Fix:** MCP tool annotations (`readOnlyHint`, `destructiveHint`, `idempotentHint`)
+on every tool. The master's MCP host now asks for approval for `destructiveHint`
+tools even on `allow` servers (`mcp.confirm_destructive`, default `true`; added in the
+review pass after [AUDIT_MCP.md](AUDIT_MCP.md)).
+
+#### MCP-S4
+**`JAVIS_SLAVE_DEVICES` parsing fragile** — *Low*
+
+Keys were not stripped (`"a=…, kitchen=…"` registered `" kitchen"`), and one malformed
+entry aborted parsing of all following ones. **Fix:** per-entry parsing with stripped
+keys and a warning per bad entry; optional token per entry.
+
+#### MCP-S5
+**Duplication and drift** — *Low*
+
+24 copies of the same try/`asyncio.run`/except block, a new `httpx.AsyncClient` per
+call, unused imports, and README entries that don't match the code
+(`slave_sleep` has no `delay`; brightness steps default to 5, not 10).
+**Fix:** one shared client per satellite (`trust_env=False`, so the token is never sent
+through an environment proxy), one `_run()` helper, README corrected.
+
+### javis-slave
+
+#### SLV-01
+**Control API has no authentication** — *Critical*
+
+`build_http_app` (http_server.py) registers `/mcp/v1/power/{sleep,shutdown,restart}`,
+mic mute, volume, brightness, engine restart and the master pass-through
+`/mcp/v1/master/slaves` without any auth, and `api_host` defaults to `0.0.0.0`.
+Reproduced: `POST /mcp/v1/power/cancel` from an unauthenticated client → **200**.
+
+* Anyone on the LAN can power off / reboot the satellite, silence its microphone or
+  read the master's full device list (names, rooms, IPs) through the pass-through,
+  which uses the satellite's own master credentials.
+* Even on loopback, any web page open on the device can trigger them: a body-less
+  `POST` is a CORS "simple request", so the browser sends it without a preflight
+  (classic CSRF).
+
+**Fix:**
+* each satellite gets a random `control_token` (generated on first start, stored in
+  its config; `--control-token` sets a household-wide one, `--show-control-token`
+  prints it, the settings dialog shows it);
+* every endpoint except `/mcp/v1/health` requires `Authorization: Bearer <token>`
+  (constant-time compare);
+* zero-config compatibility: callers on **loopback** may omit the token but must send
+  `X-Javis-Control: 1`. A browser cannot add that header cross-origin without a
+  preflight, which this server never grants, so CSRF stays blocked;
+* `javis-slave-mcp` sends the header always and the token from `JAVIS_SLAVE_TOKEN`
+  or per device.
+
+#### SLV-02
+**Volume control broken with current `pycaw`** — *High*
+
+`VolumeController` calls `AudioUtilities.GetSpeakers().Activate(IAudioEndpointVolume._iid_, 0, None)`
+and uses the result directly. Since pycaw 2024 `GetSpeakers()` returns an
+`AudioDevice` wrapper **without** `Activate` (it exposes `.EndpointVolume`); on older
+pycaw the returned `IUnknown` pointer also needed a `cast` to `IAudioEndpointVolume`.
+Reproduced: `AttributeError: 'AudioDevice' object has no attribute 'Activate'` → the
+controller disables itself and every volume endpoint fails on Windows.
+**Fix:** use `EndpointVolume` when present, else the documented
+`cast(Activate(..., CLSCTX_ALL, None), POINTER(IAudioEndpointVolume))`. The endpoint is
+resolved per call, so it follows default-device changes and no COM pointer is shared
+between `asyncio.to_thread` workers (calling from workers was verified to work).
+
+#### SLV-03
+**Turn upload / pairing retried after a read timeout** — *Medium*
+
+`_retryable_request` retries on any `httpx.HTTPError`, including `ReadTimeout` /
+`RemoteProtocolError`, i.e. after the master already received the request.
+`send_turn` (`retries=1`) then re-runs the whole STT → LLM → tools turn ("add milk to
+the list" twice), and `pair` re-submits a single-use code that was already consumed
+(the user sees "code invalid" although pairing succeeded on the master).
+**Fix:** non-idempotent calls only retry when the connection could not be established
+(`ConnectError`, `ConnectTimeout`).
+
+#### SLV-04
+**Failed engine start leaks the open microphone** — *Medium*
+
+`AudioEngine.start()` opens the mic, then builds the VAD and loads/downloads the wake
+model. If the latter fails (custom model not on the master, ONNX error), the started
+`InputStream` is never closed. Both the GUI and headless runners retry every 10 s with
+a new engine: each retry opens another stream; on a Raspberry Pi with an ALSA `hw:`
+device the second open fails with "Device or resource busy", so the satellite never
+recovers without a restart. **Fix:** `start()` stops everything it opened on failure.
+
+#### SLV-05
+**Playback thread keeps writing while the stream is closed** — *Medium*
+
+Same defect as master AGT-06: `play_pcm` awaits `asyncio.to_thread(_writer)`. Restart,
+re-pair or shutdown during a reply cancels the engine task; `to_thread` returns at
+once while the thread keeps calling `stream.write()`, and the `finally` block calls
+`stop()/close()` concurrently. **Fix:** cancellation-safe block writer that stops and
+joins the writer thread before the stream is closed.
+
+#### SLV-06
+**Master-supplied wake-word name used as a file path** — *Medium*
+
+`_load_wake` downloads to `config_dir()/models/f"{name}.onnx"` with `name` taken from
+the master's `/config` response. Over plain HTTP (SLV-17), a spoofed or compromised
+master could send `../../.config/autostart/x` and write files outside the models
+folder. **Fix:** names containing path separators or `..` are rejected.
+
+#### SLV-07
+**Corrupt config silently replaced** — *Medium*
+
+`load_config()` swallows any parse error and returns fresh defaults, including a
+**new random `device_id`**; both entry points immediately `save_config()` it. A
+truncated `slave_config.json` (SD card + power loss — `tmp.replace` without `fsync`
+is not durable) therefore wipes the pairing and the satellite reappears as a new device.
+**Fix:** unreadable files are preserved as `slave_config.json.corrupt` with an error
+log; saves `fsync` before the atomic replace; values from JSON are type-checked.
+
+#### SLV-08
+**`slave_config.json` (API key) world-readable on Linux** — *Medium*
+
+The file holds the master API key (and now the control token) but is created with the
+default umask (0644): every local user on the Pi can read it. **Fix:** `chmod 600` on
+POSIX after every write.
+
+#### SLV-09
+**Windows power commands report success regardless of exit code** — *Low*
+
+`subprocess.run(["shutdown", ...], check=False)` → always `True`, e.g.
+`/mcp/v1/power/cancel` said "cancelled" when nothing was pending (exit 1116), and a
+console window flashed in GUI mode. **Fix:** shared runner with timeout,
+`CREATE_NO_WINDOW` and return-code check (exit 1116 "no shutdown in progress" counts as
+success for *cancel*). `sleep` is started without waiting: `SetSuspendState` only
+returns after resume, which held the HTTP reply until the machine woke up.
+
+#### SLV-10
+**Retry counter double-counted; misleading error** — *Low*
+
+`_retryable_request` already calls `mark_disconnected()` per attempt; `_handle_turn`
+and `_config_poll_loop` called it again, so the counter grew twice as fast. After 10
+the UI said "Maximale Wiederverbindungsversuche erreicht. Bitte manuell neu koppeln."
+although polling continues and recovers automatically. **Fix:** single bookkeeping
+point; the message now says reconnection keeps being attempted.
+
+#### SLV-11
+**30 ms chunks zero-padded** — *Low*
+
+Same as master AUD-02. **Fix:** 32 ms (512 samples @ 16 kHz).
+
+#### SLV-12
+**Mic callback can raise during shutdown** — *Low*
+
+Same as master AUD-04. **Fix:** guarded.
+
+#### SLV-13
+**Fire-and-forget tasks** — *Low*
+
+`ensure_future()` results discarded for chimes, wake-model swaps, pairing, mute,
+HTTP-server start, engine retry. **Fix:** small `spawn()` helper keeping references
+and logging failures.
+
+#### SLV-14
+**Transcript bubbles render LLM text as rich text** — *Low*
+
+`QLabel(f"🤖 {reply_text}")` uses `Qt::AutoText`; replies containing HTML (e.g.
+echoed from web search results) are rendered, including `<img src="file:…">`.
+**Fix:** `Qt.PlainText` for user/assistant/error bubbles.
+
+#### SLV-15
+**`windows_control.py` is an unused copy** — *Info*
+
+It differs from `system_control.py` only in the logger import and is imported nowhere.
+Recommend deleting it (left in place pending your confirmation).
+
+#### SLV-16
+**uvicorn overrides the headless SIGTERM handler** — *Info*
+
+`Server.serve()` installs its own SIGINT/SIGTERM handlers after the headless runner
+registered `stop.set`. uvicorn ≥ 0.29 re-raises captured signals when it exits, so
+shutdown still works — older versions (the spec allowed `>=0.24`) would leave
+`systemctl stop` hanging until SIGKILL. **Fix:** `uvicorn>=0.29`.
+
+#### SLV-17
+**Plain HTTP; the satellite does not authenticate the master** — *Info*
+
+API key, transcripts and reply audio travel unencrypted, and anything answering on the
+master's IP is trusted (wake-word settings, model downloads). 📌 Run the master behind
+TLS (Caddy) or keep satellites on a trusted VLAN; SLV-06 limits the worst case.
+
+#### SLV-18
+**systemd unit has no sandboxing** — *Info*
+
+📌 Consider `ProtectSystem=full`, `ProtectHome=read-only`, `PrivateTmp=yes`,
+`ReadWritePaths=/opt/javis-slave/config` (sudo for `shutdown` keeps working;
+`NoNewPrivileges=yes` would not). Not applied: it can't be tested here, and
+`ProtectHome` breaks installs under `/home`; see §6 for the options that must stay off.
+
+#### LINT-S
+Unused imports (`SlaveConfig`, `Any`, `Context`, `Field`), missing exception chaining
+(B904 ×2), f-strings without placeholders (F541 ×2). **Fixed.**
+
+---
+
+## 4. Reviewed and rejected
+
+| Claim | Why rejected |
+|---|---|
+| Stateful resampler drops/duplicates samples between blocks | Phase carry-over and the `_last` sample are handled correctly; output length tracks the ratio. |
+| `MicStream.stop()` can block a consumer forever | `_drain_and_sentinel` evicts until the sentinel fits. |
+| Mute race between API and UI | `_mute_lock` + `_unmuted` event serialise it. |
+| Pairing code brute force from the satellite side | Codes are validated and rate-limited on the master (see master API audit). |
+
+---
+
+## 5. Verification
+
+| Check | Result |
+|---|---|
+| `ruff check javis-slave/src javis-slave-mcp/src --select F,E9,B` | clean (was 8 findings) |
+| `pytest tests` (main venv) | 59 passed, 1 skipped (`test_slave_mcp.py` needs mcp 1.x) |
+| `javis-slave-mcp/.venv` → `pytest tests/test_slave_mcp.py` | 5 passed |
+| Offscreen GUI smoke (`SlaveWindow`, bubbles, settings dialog) | plain-text bubbles, token field masked |
+
+New regression tests:
+
+* [tests/test_slave_audit.py](../tests/test_slave_audit.py):
+  * Config: a corrupt file is kept as `.corrupt`, loading is type-checked, and the token round-trips.
+  * Control API:
+    * LAN callers need the Bearer token.
+    * Loopback callers need `X-Javis-Control`.
+    * Plain loopback requests (CSRF) and empty tokens are refused.
+    * `/docs` is gone.
+  * Master client: `send_turn` does not retry after `ReadTimeout`, but retries a `ConnectError` once.
+  * Audio:
+    * A failed engine start releases the mic.
+    * Wake-model names with path parts are rejected before any download.
+    * Cancelled playback stops writing before the stream can be closed.
+  * System control: the pycaw `EndpointVolume` path and Windows exit-code handling.
+* [tests/test_slave_mcp.py](../tests/test_slave_mcp.py):
+  * Env parsing skips bad entries and never puts the token in `repr`.
+  * A tool called through `FastMCP.call_tool` on a running loop resolves the real UUID and sends both auth headers.
+  * 401 responses and unknown devices become tool errors.
+  * Power tools carry `destructiveHint`.
+
+No test calls a real power, volume or brightness endpoint. The controllers are faked, or the
+auth middleware rejects the request before the handler runs.
+
+---
+
+## 6. Improvements and best practices
+
+Researched after the fix pass; ✅ = applied, 📌 = recommendation.
+
+| Source | Guidance | Status |
+|---|---|---|
+| [MCP spec — Tools](https://modelcontextprotocol.io/specification/2025-06-18/server/tools) | Tool execution failures belong in the result with `isError: true`. Protocol errors are for unknown tools and bad arguments. | ✅ `ToolError` from `_run()` |
+| MCP spec — Tools | Annotations describe behaviour, but clients **MUST** treat them as untrusted unless the server is trusted. A human **SHOULD** be able to deny invocations. | ✅ Annotations on all 23 tools. ✅ The master's MCP host asks before `destructiveHint` tools, even on `approval: allow` servers. |
+| MCP spec — Tools, security | Servers **MUST** validate inputs and implement access control, and **SHOULD** rate-limit. | ✅ FastAPI `Query(ge/le)` bounds + auth. 📌 No rate limit on power endpoints yet. |
+| [OWASP CSRF cheat sheet](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html) | Custom request headers are a valid CSRF defence for APIs because they force a CORS preflight. Never change state via `GET`. Compare secrets in constant time. | ✅ `X-Javis-Control` on loopback, no CORS middleware, all state changes use `POST`, `hmac.compare_digest`. 📌 Optionally also reject `Sec-Fetch-Site: cross-site`. |
+| [uvicorn 0.29.0 release](https://github.com/encode/uvicorn/releases/tag/0.29.0) | "Cooperative signal handling" (#1600): captured signals are re-raised after shutdown. | ✅ `uvicorn>=0.29` |
+| [pycaw README](https://github.com/AndreMiras/pycaw) | Current API: `AudioUtilities.GetSpeakers().EndpointVolume`. | ✅ Used, with an `Activate`+`cast` fallback for old pycaw |
+| [systemd.exec(5)](https://man7.org/linux/man-pages/man5/systemd.exec.5.html) | Enable `ProtectSystem=` for all long-running services and `ProtectHome=` for network-facing ones. `NoNewPrivileges=` blocks setuid, `ProtectKernelTunables=` makes `/sys` read-only, and `PrivateDevices=` removes physical devices. | 📌 Safe set for this unit: `ProtectSystem=full`, `PrivateTmp=yes`, `ProtectHome=read-only` (only for `/opt` installs). These must stay **off**: `NoNewPrivileges` (sudo power control), `ProtectKernelTunables` (backlight), `PrivateDevices` (audio). |
+| systemd.exec(5), *Environment=* | Environment variables are not suitable for secrets: they are visible via D-Bus and inherited by child processes. | 📌 For remote satellites, prefer `"token"` in `~/.javis/slave_mcp_config.json` (keep it 0600) over `JAVIS_SLAVE_TOKEN` in `mcp_servers.json`. |
+| Python `hmac.compare_digest` / `secrets.token_urlsafe` | Use constant-time comparison and CSPRNG tokens. | ✅ 24-byte URL-safe token, constant-time check |
+
+Observations (no action):
+
+* `mcp` 1.29 with the current `pydantic-settings` prints an `IncompleteFieldDefinitionWarning`
+  (`lifespan`) when `FastMCP()` is constructed. This is an upstream SDK issue and harmless.
+* `mcp` 2.x removed `mcp.server.fastmcp`, so `javis-slave-mcp` now pins `mcp>=1.10,<2.0`
+  (tool annotations need ≥ 1.10). The master's MCP host already supports both.
+* Still open:
+  * SLV-15: delete `windows_control.py`?
+  * SLV-17: TLS.
+  * SLV-18: sandboxing.
+  * Rate limiting.