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

420 lines
22 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.
# Homey, Emby, DuckDuckGo & Spotify MCP Servers — Code Audit
| | |
|---|---|
| **Scope** | `homey-mcp` (Python, ~1.6k lines), `emby-mcp` (Python, ~0.6k), `duckduckgo-mcp` (Python, ~0.2k), `spotify-mcp-server` (TypeScript, ~2.5k) |
| **Method** | Every file read by hand. Each finding was reproduced: offline with mocked HTTP transports, or with **read-only** live requests against the configured Homey, Emby and Spotify accounts. Nothing was switched, played, deleted or changed during the audit. |
| **Date** | 2026-10-03 |
| **Companion audits** | [AUDIT.md](AUDIT.md) (Javis), [AUDIT_SLAVE.md](AUDIT_SLAVE.md) (satellite + slave MCP) |
---
## 1. How Javis uses these servers
All four run as **stdio** subprocesses of the Javis MCP host (`mcp_servers.json`).
`homey-mcp` can additionally run as a LAN HTTP server (streamable-http / SSE + a REST
API) for other clients such as Odysseus.
| Server | Approval in `mcp_servers.json` | Credential it holds |
|---|---|---|
| homey | `ask` (default) | Homey Pro personal access token |
| emby | `allow` | Emby **admin** API key |
| duckduckgo | `allow` | — |
| spotify | `ask` | OAuth client secret + refresh token |
The Javis host treats a tool result as failed only when MCP reports `isError: true`, which
it turns into an `ERROR:` prefix. Failures that are returned as ordinary text therefore count
as **successes**: they are cached, recorded as quick commands and reported as done.
---
## 2. Findings summary
| ID | Severity | Server | Finding | Status |
|---|---|---|---|---|
| [EMB-01](#emb-01) | Critical | emby | All Emby tools fail with Javis' config: the scheme defaults to `https` while the server speaks HTTP | Fixed |
| [EMB-02](#emb-02) | High | emby | `search_items` only looks at the first 60 of 68,932 library items | Fixed |
| [EMB-03](#emb-03) | High | emby | `play_item`, `get_item`, `playback_url` call endpoints that don't exist; `list_devices` always returns `[]` | Fixed |
| [EMB-04](#emb-04) | High | emby / security | IDs inserted unencoded into URL paths, with the admin key (`123/Delete`, `../System/Restart`) | Fixed |
| [HOM-01](#hom-01) | High | homey / security | Path injection through IDs: `trigger_flow('../../system/reboot#')` reboots the Homey | Fixed |
| [HOM-02](#hom-02) | High | homey LAN / security | `AUTH_MODE=bearer` leaves the SSE endpoints `/sse` and `/messages/` unauthenticated | Fixed |
| [HOM-03](#hom-03) | Medium | homey LAN / security | DNS-rebinding protection disabled although the docs promise a Host check | Fixed |
| [HOM-04](#hom-04) | Medium | homey | Realtime events never arrive (wrong Socket.IO protocol) | Fixed |
| [HOM-05](#hom-05) | Medium | homey | Zone names resolve to the first substring hit (`Wohnzimmer` can pick `Wohnzimmer Ecke`) | Fixed |
| [HOM-06](#hom-06) | Medium | homey | `find_devices` matches ANY query word, so "esszimmer licht" returns every light | Fixed |
| [DDG-01](#ddg-01) | Medium | duckduckgo | Blocking searches run on the server's event loop | Fixed |
| [DDG-02](#ddg-02) | Medium | duckduckgo | Search failures are returned as normal text, so Javis caches them as results | Fixed |
| [SPO-01](#spo-01) | Medium | spotify | Failures are returned without `isError` in all 28 tools | Fixed |
| [SPO-02](#spo-02) | Medium | spotify | `playMusic(uri="spotify:track:…")` is sent as a context URI and rejected by Spotify | Fixed |
| [SPO-03](#spo-03) | Medium | spotify | Token refresh ignores rotated refresh tokens, has no expiry margin and can run in parallel | Fixed |
| [EMB-05](#emb-05) | Medium | emby | Failures are returned as `{"ok": false}` data instead of tool errors | Fixed |
| [HOM-07](#hom-07) | Low | homey | systemd unit cannot work (`homey-mcp@user` enabled, but not a template; `%h` is `/root`) | Fixed |
| [HOM-08](#hom-08) | Low | homey | Errors returned as data; REST returns 500 on bad JSON; non-ASCII token crashes the auth check | Fixed |
| [EMB-06](#emb-06) | Low | emby | Wrong field mapping (runtime, year, episode numbers); documented `type` filter is missing | Fixed |
| [DDG-03](#ddg-03) | Low | duckduckgo | No input validation | Fixed |
| [SPO-04](#spo-04) | Low | spotify | Credentials file written non-atomically with default permissions | Fixed |
| [SPO-05](#spo-05) | Low | spotify | IDs unencoded in raw API paths | Fixed |
| [SPO-06](#spo-06) | Low | spotify | `adjustVolume` reads one device's volume and sets another's | Fixed |
| [SPO-07](#spo-07) | Low | spotify | `3:60` durations, playlist search lines, saved-track numbering | Fixed |
| [X-01](#x-01) | Low | all | No MCP tool annotations (e.g. `control_all` can switch off every socket) | Fixed |
| [X-02](#x-02) | Low | Python servers | `mcp>=1.2` allows mcp 2.x, which removed `mcp.server.fastmcp` | Fixed (pin) |
| [X-03](#x-03) | Info | all | READMEs out of date (tool lists, defaults, paths) | Fixed |
| [X-04](#x-04) | Info | homey / emby / spotify | Broad credentials (admin Emby key, full Homey token, unused Spotify scopes) | Open (recommendation) |
| [X-05](#x-05) | Info | spotify | Biome reports format errors (CRLF line endings) — present before the audit | Open |
---
## 3. Detailed findings
### Emby
#### EMB-01
**All Emby tools fail with Javis' configuration** — *Critical*
`mcp_servers.json` sets `EMBY_HOST=10.10.10.50:9096` without a scheme. `Settings.scheme`
defaulted to `https`, so the base URL became `https://10.10.10.50:9096`. That port speaks
plain HTTP:
```
https://10.10.10.50:9096/System/Info/Public → ConnectError [SSL: WRONG_VERSION_NUMBER]
http://10.10.10.50:9096/System/Info/Public → 200 {"ServerName": …}
```
`emby-mcp/.env` (scheme `https`, port 443) is **not** read when Javis launches the server,
because pydantic-settings resolves `.env` relative to the working directory, which is
`F:\Javis`.
**Fix:** `scheme` defaults to `http` (Emby's own default). `EMBY_HOST` may now be
`host:port` or a full URL, and `EMBY_PORT` is ignored when the host already has a port.
The README explains the `.env` lookup.
#### EMB-02
**Search only sees the first 60 items** — *High*
`search_items` fetched `limit*3` items sorted by name and filtered them in Python. The library
holds 68,932 items, so almost every title was out of reach. **Fix:** server-side
`SearchTerm` plus `IncludeItemTypes`. Without the type filter Emby also returns People,
Studios and other non-media items. Verified live: the fixed search finds titles across the
whole library.
#### EMB-03
**Endpoints that don't exist** — *High*
Checked against the server's own `/openapi.json` and live requests:
| Tool | Called | Reality |
|---|---|---|
| `get_item` | `GET /Items/{id}` | only `DELETE` exists → 404 |
| `play_item` | `POST /Items/{id}/Play` | no such route; remote play is `POST /Sessions/{id}/Playing?ItemIds=…&PlayCommand=PlayNow` |
| `playback_url` | `GET /Items/{id}/StreamUrl` | no such route; streams are `/Videos/{id}/stream`, `/Audio/{id}/stream` |
| `list_devices` | `GET /Devices` expecting a list | returns `{"Items": [...]}` → tool always `[]` |
**Fix:**
- `get_item` uses `/Items?Ids=…` with detail fields.
- `play_item` picks a remote-controllable session by `session_id` or a `device` name, or the
only one there is, and errors with the list of candidates otherwise.
- `list_sessions` exposes `id`, `supports_remote_control` and `now_playing`.
- `playback_url` builds the static stream URL **without** the API key.
- `list_devices` unwraps `Items`.
#### EMB-04
**IDs inserted unencoded into URL paths** — *High (security)*
The server authenticates with the **admin** API key, and Javis approves Emby tools
automatically (`approval: allow`). The spec lists `POST /Items/{Id}/Delete`,
`/System/Restart` and `/System/Shutdown`. An item ID like `123/Delete?` in `play_item`
became `POST /Items/123/Delete`, deleting the media item. `../System/Restart#` restarted the
server. Such an ID could come from prompt injection, for example via web search results or
media metadata. **Fix:** every path parameter is percent-encoded as a single segment, and
`get_item` passes the ID as a query value. Verified: `123/Delete` now yields a harmless
lookup error.
#### EMB-05
**Failures returned as data** — *Medium*
`ping`, `playback_url`, `playback_info` and `play_item` caught exceptions and returned
`{"ok": false, "error": …}`. MCP reports those as successful results. **Fix:** they raise,
so FastMCP returns `isError: true`. `ping` keeps its `ok` flag because it is a probe.
#### EMB-06
**Field mapping** — *Low*
`RunTimeNumeric`, `SeasonNumber`, `EpisodeNumber` and `Year` are not Emby fields, so
runtime, year and episode numbers were always empty. The instructions advertised
`list_items(type='Movie')`, but no such parameter existed. A `None` name crashed the pydantic
model. **Fix:**
- Runtime is computed from `RunTimeTicks`.
- Season and episode numbers come from `ParentIndexNumber` and `IndexNumber`.
- `ProductionYear` is requested through `Fields`.
- New `item_type` parameter.
- Aliases removed.
### Homey
#### HOM-01
**Path injection through IDs** — *High (security)*
`homey_client` interpolated `device_id`, `flow_id`, `variable_id` and `capability_id` into
URL paths. httpx normalises dot segments and drops `#…` fragments. Reproduced offline:
```
trigger_flow('../../system/reboot#') → POST /api/manager/system/reboot
set_variable('../../../system/reboot#', 1) → PUT /api/system/reboot
```
**Fix:** IDs are percent-encoded single path segments (`_seg`).
#### HOM-02
**Bearer mode did not protect the SSE transport** — *High (security, LAN mode)*
The auth middleware only checked `/mcp` and the root alias. With
`HOMEY_MCP_TRANSPORT=sse` and `HOMEY_MCP_AUTH_MODE=bearer`, both `GET /sse` and
`POST /messages/` went through unauthenticated. **Fix:** in bearer mode every route
requires the token except `GET /` (discovery) and `/openapi.json`. The token is compared as
bytes, so a non-ASCII header no longer raises inside `compare_digest`.
#### HOM-03
**No DNS-rebinding protection** — *Medium (security, LAN mode)*
FastMCP's Host/Origin validation was switched off (`enable_dns_rebinding_protection=False`),
yet README and UBUNTU_SETUP promise a Host check (421) via
`HOMEY_MCP_ADVERTISED_HOST`. In the default `AUTH_MODE=none`, any web page opened in **any
browser on the LAN** could rebind its domain to the server and control the smart home over
MCP. Without a token the REST API was open the same way. **Fix:** a Host-header allow-list
covers MCP **and** REST and answers `421 Misdirected Request`. It is built from the
advertised host, `localhost`, `127.0.0.1` and `HOMEY_MCP_ALLOWED_HOSTS` (`*` turns it off).
If the bind address is a wildcard and nothing is configured, the server logs a warning.
#### HOM-04
**Realtime events never arrived** — *Medium*
The client connected with `auth={"token"}` and listened for `device.update` on the root
namespace. Athom's `homey-api` (`SocketSession.js`, `SubscriptionRegistry.js`) shows the real
protocol:
1. `handshakeClient {token, homeyId}` returns a namespace (`/api`).
2. Connect to that namespace.
3. `subscribe` to URIs such as `homey:manager:devices` and `homey:device:<id>`.
4. Events arrive named by their URI, with `(event, data)` as payload.
Verified live (read-only): the old code received 0 events. **Fix:** the client implements the
handshake and subscriptions, rebuilds them on reconnect, and buffers compact entries. The
fixed client captured 18 `capability` events in 25 s.
#### HOM-05
**Zone resolution by first substring hit** — *Medium*
`control_zone`, `get_zone_status` and the REST `zones/control` took the first zone whose
name *contained* the query. That depends on dict order: "Wohnzimmer" could switch
"Wohnzimmer Ecke". `control_zone` also matched devices of *other* zones by name substring
when the zone was embedded as an object. **Fix:** a shared `_resolve_zone` prefers an exact
match, then a prefix, then a substring (shortest name wins). Devices are matched by zone ID
only.
#### HOM-06
**`find_devices` too broad** — *Medium*
The query was split into words and a device matched if **any** word hit its name or zone.
"esszimmer licht" therefore returned every light in the house plus every Esszimmer device,
and the model then acts on one of them. **Fix:** all words must match. Only if nothing does
are partial matches returned.
#### HOM-07
**systemd unit** — *Low*
`homey-mcp.service` uses `User=%i`, but `setup-ubuntu.sh` copies it as `homey-mcp.service`
and enables `homey-mcp@$USER.service`, which fails because the template doesn't exist. In
addition, `%h` resolves to `/root` for system units: specifiers ignore `User=`. **Fix:** the
unit is documented and installed as the template `homey-mcp@.service` and uses
`/home/%i/…`.
#### HOM-08
**Error handling** — *Low*
The following returned errors as data instead of raising:
- `control_all`'s safety refusal (`{"error": …}`)
- `get_temperatures` (`[{"error": …}]`)
- `send_notification` (`{"ok": false}`)
In addition, REST handlers returned 500 on invalid JSON. **Fix:** the tools raise, and REST
returns 400.
### DuckDuckGo
#### DDG-01
**Blocking calls on the event loop** — *Medium*
The tools were plain `def` functions. FastMCP calls those directly on its event loop, and a
search took 0.5–2 s (measured). During that time the server could not answer anything else,
including cancellations. **Fix:** the tools are `async` and run `ddgs` in
`asyncio.to_thread`.
#### DDG-02
**Failures look like results** — *Medium*
Rate limits and network errors came back as `"Search failed: …"` text. The Javis host
caches non-`ERROR:` results of cacheable tools, so the failure was replayed for later
identical queries. **Fix:** failures raise `ToolError` (`isError: true`).
#### DDG-03
**Input validation** — *Low*
An empty query raised `DDGSException: query is mandatory` from deep inside the library, and
`safesearch` accepted any value. **Fix:** both are validated up front.
### Spotify
#### SPO-01
**No `isError`** — *Medium*
Every handler caught exceptions and returned `"Error …"` text as a normal result. For Javis,
"Error starting playback: 404 No active device" was a success. **Fix:** a single wrapper in
`index.ts` marks results whose text starts with `Error` and turns thrown errors into
`isError: true`. `adjustVolume`'s "No active device" messages now follow the same
convention. `verify_tools.mjs` counts `isError` as a failure.
#### SPO-02
**Track URIs played as context** — *Medium*
`playMusic` checked only `type === 'track'`. With just `uri: "spotify:track:…"`, which LLMs
often send, the track went into `context_uri` and Spotify rejected it. **Fix:** the kind is
taken from the URI; tracks and episodes go to `uris`.
#### SPO-03
**Token refresh** — *Medium*
Spotify's refresh response may contain a new `refresh_token`
([docs](https://developer.spotify.com/documentation/web-api/tutorials/refreshing-tokens)),
and the code discarded it. Refresh happened only *after* expiry, so a token could expire
mid-request. Concurrent tool calls each refreshed and wrote the file. An expired 6-month
refresh token (`invalid_grant`) produced an opaque error. **Fix:**
- Single-flight refresh, 60 s before expiry.
- A rotated refresh token is saved.
- `invalid_grant` produces the message "refresh token expired or revoked — run
`npm run auth`".
#### SPO-04 – SPO-07
- **SPO-04** `spotify-config.json` (client secret and refresh token) is now written via
temp-file + rename with mode `0600`.
- **SPO-05** Playlist and user IDs are `encodeURIComponent`-ed in raw API paths. Verified:
`../me` stays inside `playlists/…`.
- **SPO-06** `adjustVolume` reads the volume of the device it is about to change.
- **SPO-07** Display fixes:
- `formatDuration(179600)` used to print `2:60`.
- Playlist search lines printed the description as a track count.
- Saved-track numbering ignored the offset for removed items.
### Cross-cutting
#### X-01
**Tool annotations** — *Low*
All tools now carry MCP annotations:
- `readOnlyHint` for lookups.
- `destructiveHint` for:
- Homey's `control_zone`, `control_all` and `trigger_flow`.
- Spotify's removals, playlist edits and reordering.
- `openWorldHint` for web and Spotify calls.
The Javis host now asks before any tool marked `destructiveHint` (and not `readOnlyHint`),
even on servers with `approval: allow`. In practice this covers the satellite power tools.
Turn it off with `mcp.confirm_destructive: false`.
#### X-02
**Dependency pins** — *Low*
`mcp[cli]>=1.2` would install mcp 2.x, which no longer ships `mcp.server.fastmcp`. All three
servers now pin `mcp[cli]>=1.13,<2`. `ddgs>=9.0` is pinned for the renamed package.
#### X-03
**Docs** — *Info*
The READMEs listed 8 of 21 Homey tools, claimed DuckDuckGo "instant answers", gave Emby's
default scheme as https and referenced `d:\Javis` paths. All four READMEs are updated.
#### X-04
**Least privilege** — *Info*
- The Emby admin API key can delete media and restart or shut down the server.
- The Homey personal access token usually carries all scopes.
- Spotify requests `user-read-email`, which no tool uses.
📌 See §6.
#### X-05
**Biome formatting** — *Info*
`npx biome check` reports 8 format errors caused by CRLF line endings. These existed before
the audit. `biome lint` and `tsc` are clean.
---
## 4. Reviewed and rejected
| Claim | Why rejected |
|---|---|
| Spotify `handleSpotifyRequest` swallows real errors ("Unexpected token") | The SDK validates the HTTP status before parsing; only successful non-JSON bodies (player endpoints) reach the parser. |
| Homey `ping` should raise | It is a connectivity probe; `ok: false` with the reason is the intended result. |
---
## 5. Verification
| Check | Result |
|---|---|
| `homey-mcp/.venv` → `pytest tests/test_homey_mcp.py` | 9 passed |
| `emby-mcp/.venv` → `pytest tests/test_emby_mcp.py` | 8 passed |
| `duckduckgo-mcp/.venv` → `pytest tests/test_duckduckgo_mcp.py` | 4 passed |
| `ruff check …/src` + new tests (`F,E9,B`) | clean |
| Spotify `tsc --noEmit`, `biome lint`, `npm run build` | clean |
| Live, read-only: Emby | ping, search, `get_item`, `playback_url` (no key in URL), users, sessions, devices all OK; crafted ID `123/Delete` → harmless lookup error |
| Live, read-only: Homey realtime | handshake + subscriptions, 18 events in 25 s (before: 0) |
| Live, read-only: Spotify over stdio | 28 tools (12 read-only, 5 destructive); search OK; bad and crafted playlist IDs → `isError: true`, path stays encoded |
The new tests:
- **Homey:**
- Encoded IDs.
- Zone ranking.
- All-words search.
- `control_zone` scoped by ID.
- `control_all` refusal.
- Compact events.
- Bearer protection of `/sse` and `/messages/` and the root alias.
- Host guard (421).
- Annotations.
- **Emby:**
- Base-URL forms.
- Encoded IDs.
- Server-side search parameters.
- `get_item` via `/Items?Ids`.
- Session-targeted play.
- Device unwrapping.
- Key-free stream URL.
- Annotations.
- **DuckDuckGo:**
- Search runs in a worker thread.
- Rate limits become `ToolError`.
- Input validation.
- Annotations.
---
## 6. Improvements and best practices
✅ = 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`. | ✅ All four servers (EMB-05, HOM-08, DDG-02, SPO-01) |
| MCP spec — Tools | Annotations describe behaviour; a human should be able to deny invocations. | ✅ Annotations everywhere. ✅ The Javis host asks before `destructiveHint` tools even on `approval: allow` servers (`mcp.confirm_destructive`, default on). |
| [MCP spec — Transports, security warning](https://modelcontextprotocol.io/specification/2025-06-18/basic/transports) | Servers **MUST** validate `Origin`/Host against DNS rebinding, **SHOULD** bind to localhost when local, **SHOULD** authenticate all connections. | ✅ Host guard + auth on every MCP route (HOM-02/03). 📌 Prefer `HOMEY_MCP_AUTH_MODE=bearer` wherever the client can send a header. |
| [MCP — Security best practices](https://modelcontextprotocol.io/specification/2025-06-18/basic/security_best_practices) | Local servers: use stdio or require a token on HTTP. Scope minimisation: least-privilege tokens. | ✅ Javis uses stdio for all four. 📌 Homey: create a PAT with only the scopes the tools need (devices, zones, flows, logic, notifications). Emby: consider a restricted user's token instead of the admin API key, or keep Emby on `approval: ask`. Spotify: drop the unused `user-read-email` scope at the next `npm run auth`. |
| OWASP Injection / path traversal guidance | Never build URLs by concatenating untrusted input; encode path parameters. | ✅ Homey, Emby, Spotify (HOM-01, EMB-04, SPO-05) |
| [Spotify — Refreshing tokens](https://developer.spotify.com/documentation/web-api/tutorials/refreshing-tokens) | Store a rotated refresh token; `invalid_grant` means re-authorize; refresh tokens last 6 months. | ✅ SPO-03 |
| Athom `homey-api` (`SocketSession.js`, `SubscriptionRegistry.js`) | Handshake → namespace → URI subscriptions; re-subscribe after reconnect. | ✅ HOM-04 |
| Emby server OpenAPI (`/openapi.json`) | Use documented routes (`/Sessions/{Id}/Playing`, `/Items?Ids=`, `SearchTerm`). | ✅ EMB-02/03 |
| Python asyncio / FastMCP | Never block the event loop; offload sync I/O to threads. | ✅ DDG-01 |
| OWASP LLM prompt-injection guidance | Treat tool output from the web as untrusted data. | ✅ DuckDuckGo server instructions say so. 📌 Same idea for media and device names returned by Emby and Homey. |
| systemd.exec(5) | Specifiers like `%h` describe the service manager's user, not `User=`. | ✅ HOM-07 |
Observations:
- `mcp` 2.x removed FastMCP, so these servers stay on 1.x until they are ported to the new
`MCPServer` API.
- Still open:
- Least-privilege credentials (X-04).
- CRLF line endings in the Spotify sources (X-05).