Files
javis/docs/AUDIT_SLAVE.md
T
2026-10-04 15:31:44 +02:00

387 lines
21 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters
This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.
# 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.