Skip to content

Commit 73bc5f0

Browse files
amccats3409claude
andcommitted
fix: tolerate read-only filesystem when locating tool config (#1125)
`:latest` crashed at startup under hardened Docker setups (`read_only: true`, `user: 1000:1000`) because `_get_config_path()` did an unconditional `mkdir(parents=True, exist_ok=True)` on `Path.home() / ".ha-mcp"`. Two contributing factors: 1. The Dockerfile didn't set `ENV HOME`, so under `USER mcpuser` Docker left `HOME=/`. `Path.home()` resolved to `/`, ha-mcp tried to mkdir `/.ha-mcp`, and `read_only: true` made that fatal (issue #1125). 2. Even on writable filesystems this silently polluted the container's filesystem root with a `/.ha-mcp/` directory. Three coordinated changes: - `settings_ui.py`: honor `HA_MCP_CONFIG_DIR` env var, and wrap the home-dir mkdir in `try/except OSError` so we fall back to a tmpdir path instead of crashing. Mirrors the existing pattern in `utils/usage_logger.py:120-127`. - `Dockerfile`: set `ENV HOME=/home/mcpuser` so `Path.home()` resolves correctly when running as the default user (default-user setups now persist settings to `~/.ha-mcp` instead of `/.ha-mcp`). - `Dockerfile`: chmod `/home/mcpuser` to 0755 so users that override `--user UID:GID` can still stat the directory; otherwise FastMCP's startup version-check raises `PermissionError` on `~/.local/share/fastmcp/version_cache.json`. Verified locally with three Docker scenarios: bondskin's hardened compose (`--read-only --user 1000:1000 --tmpfs /tmp`), default user, and `HA_MCP_CONFIG_DIR=/data/ha-mcp` bind-mount. Server starts cleanly in all three; original stack trace from #1125 no longer reproduces. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 29397dc commit 73bc5f0

3 files changed

Lines changed: 218 additions & 48 deletions

File tree

Dockerfile

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,13 @@ LABEL org.opencontainers.image.title="Home Assistant MCP Server" \
3131
org.opencontainers.image.licenses="MIT" \
3232
io.modelcontextprotocol.server.name="io.github.homeassistant-ai/ha-mcp"
3333

34-
# Create non-root user for security
35-
RUN groupadd -r mcpuser && useradd -r -g mcpuser -m mcpuser
34+
# Create non-root user for security. /home/mcpuser is mode 0755 (instead of
35+
# the useradd default 0700) so users that override `--user UID:GID` can still
36+
# stat the home dir — otherwise FastMCP's startup version-check raises
37+
# PermissionError on Path.home()/.local/share/fastmcp/version_cache.json.
38+
RUN groupadd -r mcpuser \
39+
&& useradd -r -g mcpuser -m mcpuser \
40+
&& chmod 0755 /home/mcpuser
3641

3742
WORKDIR /app
3843

@@ -43,6 +48,12 @@ COPY --chown=mcpuser:mcpuser fastmcp.json fastmcp-http.json ./
4348

4449
USER mcpuser
4550

51+
# Set HOME explicitly. Docker doesn't auto-derive HOME from /etc/passwd when
52+
# a USER directive is set (moby/moby#2968), leaving HOME=/ at runtime. That
53+
# made Path.home() resolve to "/" and ha-mcp tried to mkdir "/.ha-mcp" on
54+
# every start — fatal under `read_only: true` (issue #1125).
55+
ENV HOME=/home/mcpuser
56+
4657
# Activate virtual environment via PATH
4758
ENV PATH="/app/.venv/bin:$PATH"
4859

src/ha_mcp/settings_ui.py

Lines changed: 114 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
import json
1313
import logging
1414
import os
15+
import tempfile
1516
from pathlib import Path
1617
from typing import TYPE_CHECKING, Any
1718

@@ -112,12 +113,62 @@ def _is_addon() -> bool:
112113

113114

114115
def _get_config_path() -> Path:
115-
"""Return the path to the tool config JSON file."""
116+
"""Return the path to the tool config JSON file.
117+
118+
Priority:
119+
1. ``HA_MCP_CONFIG_DIR`` env var — explicit override, e.g. for hardened
120+
Docker setups bind-mounting a writable volume into a ``read_only: true``
121+
container.
122+
2. ``/data/tool_config.json`` — Home Assistant add-on (writable supervisor
123+
data dir).
124+
3. ``~/.ha-mcp/tool_config.json`` — standard. Falls back to
125+
``<tempdir>/ha-mcp/tool_config.json`` when the home dir can't be
126+
created (read-only filesystem, or ``HOME`` unset so ``Path.home()``
127+
resolves to ``/``). The fallback loses persistence across restarts
128+
but lets the server start; users wanting persistence should set
129+
``HA_MCP_CONFIG_DIR``.
130+
"""
131+
config_dir_env = os.environ.get("HA_MCP_CONFIG_DIR")
132+
if config_dir_env:
133+
custom_dir = Path(config_dir_env)
134+
try:
135+
custom_dir.mkdir(parents=True, exist_ok=True)
136+
except OSError:
137+
logger.warning(
138+
"HA_MCP_CONFIG_DIR=%s is not writable; tool config will not persist.",
139+
custom_dir,
140+
)
141+
return custom_dir / "tool_config.json"
142+
116143
if _is_addon():
117144
return Path("/data") / "tool_config.json"
145+
118146
home_dir = Path.home() / ".ha-mcp"
119-
home_dir.mkdir(parents=True, exist_ok=True)
120-
return home_dir / "tool_config.json"
147+
try:
148+
home_dir.mkdir(parents=True, exist_ok=True)
149+
return home_dir / "tool_config.json"
150+
except OSError:
151+
# Home is unwritable — typically a Docker container with
152+
# `read_only: true` (issue #1125) or one where USER is set without
153+
# ENV HOME so Path.home() resolves to "/". Fall back to a tmpdir
154+
# path so the server still starts.
155+
fallback = Path(tempfile.gettempdir()) / "ha-mcp"
156+
try:
157+
fallback.mkdir(parents=True, exist_ok=True)
158+
except OSError:
159+
logger.warning(
160+
"Tool config fallback dir %s is also not writable; "
161+
"settings persistence is disabled.",
162+
fallback,
163+
)
164+
logger.warning(
165+
"Cannot write tool config to %s (read-only filesystem or HOME unset). "
166+
"Falling back to %s — settings will NOT persist across restarts. "
167+
"Set HA_MCP_CONFIG_DIR to a writable path for persistence.",
168+
home_dir,
169+
fallback,
170+
)
171+
return fallback / "tool_config.json"
121172

122173

123174
def load_tool_config(settings: Settings | None = None) -> dict[str, Any]:
@@ -166,7 +217,9 @@ def save_tool_config(config: dict[str, Any]) -> None:
166217
logger.exception("Failed to save tool config to %s", path)
167218

168219

169-
async def _get_tool_metadata(server: HomeAssistantSmartMCPServer) -> list[dict[str, Any]]:
220+
async def _get_tool_metadata(
221+
server: HomeAssistantSmartMCPServer,
222+
) -> list[dict[str, Any]]:
170223
"""Extract metadata for all registered tools from the server.
171224
172225
Uses FastMCP's internal ``local_provider._list_tools()`` because the
@@ -196,14 +249,16 @@ async def _get_tool_metadata(server: HomeAssistantSmartMCPServer) -> list[dict[s
196249
title = getattr(tool, "title", None) or tool.name
197250
if tool.annotations and getattr(tool.annotations, "title", None):
198251
title = tool.annotations.title
199-
tools.append({
200-
"name": tool.name,
201-
"title": title,
202-
"description": (tool.description or "")[:200],
203-
"tags": tags,
204-
"primary_tag": primary,
205-
"annotations": annotations,
206-
})
252+
tools.append(
253+
{
254+
"name": tool.name,
255+
"title": title,
256+
"description": (tool.description or "")[:200],
257+
"tags": tags,
258+
"primary_tag": primary,
259+
"annotations": annotations,
260+
}
261+
)
207262

208263
# Inject stub entries for feature-gated tools that aren't registered
209264
registered_names = {t["name"] for t in tools}
@@ -215,15 +270,17 @@ async def _get_tool_metadata(server: HomeAssistantSmartMCPServer) -> list[dict[s
215270
stub_annotations["readOnlyHint"] = True
216271
if meta.get("destructiveHint") == "true":
217272
stub_annotations["destructiveHint"] = True
218-
tools.append({
219-
"name": name,
220-
"title": meta["title"],
221-
"description": meta["description"],
222-
"tags": [meta["primary_tag"]],
223-
"primary_tag": meta["primary_tag"],
224-
"annotations": stub_annotations,
225-
"disabled_by": meta["disabled_by"],
226-
})
273+
tools.append(
274+
{
275+
"name": name,
276+
"title": meta["title"],
277+
"description": meta["description"],
278+
"tags": [meta["primary_tag"]],
279+
"primary_tag": meta["primary_tag"],
280+
"annotations": stub_annotations,
281+
"disabled_by": meta["disabled_by"],
282+
}
283+
)
227284

228285
tools.sort(key=lambda t: (t["primary_tag"], t["name"]))
229286
return tools
@@ -271,7 +328,8 @@ def apply_tool_visibility(
271328
return pinned_names
272329

273330

274-
_SETTINGS_HTML = """\
331+
_SETTINGS_HTML = (
332+
"""\
275333
<!DOCTYPE html>
276334
<html lang="en">
277335
<head>
@@ -434,8 +492,12 @@ def apply_tool_visibility(
434492
}
435493
}
436494
437-
const DEFAULT_PINNED = """ + json.dumps(list(DEFAULT_PINNED_TOOLS)) + """;
438-
const MANDATORY = """ + json.dumps(list(MANDATORY_TOOLS)) + """;
495+
const DEFAULT_PINNED = """
496+
+ json.dumps(list(DEFAULT_PINNED_TOOLS))
497+
+ """;
498+
const MANDATORY = """
499+
+ json.dumps(list(MANDATORY_TOOLS))
500+
+ """;
439501
440502
function getState(name) {
441503
if (toolStates[name]) return toolStates[name];
@@ -672,6 +734,7 @@ def apply_tool_visibility(
672734
</body>
673735
</html>
674736
"""
737+
)
675738

676739

677740
def register_settings_routes(
@@ -766,15 +829,18 @@ async def _save_tools(request: Request) -> JSONResponse:
766829
pinned_count = sum(1 for s in states.values() if s == "pinned")
767830
logger.info(
768831
"Saved tool config (restart required to apply): %d disabled, %d pinned",
769-
disabled_count, pinned_count,
832+
disabled_count,
833+
pinned_count,
770834
)
771835

772-
return JSONResponse({
773-
"success": True,
774-
"disabled": disabled_count,
775-
"pinned": pinned_count,
776-
"restart_required": True,
777-
})
836+
return JSONResponse(
837+
{
838+
"success": True,
839+
"disabled": disabled_count,
840+
"pinned": pinned_count,
841+
"restart_required": True,
842+
}
843+
)
778844

779845
async def _restart_addon(_: Request) -> JSONResponse:
780846
token = os.environ.get("SUPERVISOR_TOKEN")
@@ -822,9 +888,11 @@ async def _restart_addon(_: Request) -> JSONResponse:
822888
return JSONResponse({"success": True, "message": "Restart initiated"})
823889

824890
async def _settings_info(_: Request) -> JSONResponse:
825-
return JSONResponse({
826-
"is_addon": _is_addon(),
827-
})
891+
return JSONResponse(
892+
{
893+
"is_addon": _is_addon(),
894+
}
895+
)
828896

829897
secret_prefix = secret_path.rstrip("/") if secret_path else ""
830898
is_addon = _is_addon()
@@ -856,7 +924,15 @@ async def _settings_info(_: Request) -> JSONResponse:
856924
# endpoint. The frontend uses relative fetches (./api/settings/...)
857925
# so the JS works at either prefix unchanged.
858926
mcp.custom_route(f"{secret_prefix}/settings", methods=["GET"])(_settings_page)
859-
mcp.custom_route(f"{secret_prefix}/api/settings/tools", methods=["GET"])(_get_tools)
860-
mcp.custom_route(f"{secret_prefix}/api/settings/tools", methods=["POST"])(_save_tools)
861-
mcp.custom_route(f"{secret_prefix}/api/settings/restart", methods=["POST"])(_restart_addon)
862-
mcp.custom_route(f"{secret_prefix}/api/settings/info", methods=["GET"])(_settings_info)
927+
mcp.custom_route(f"{secret_prefix}/api/settings/tools", methods=["GET"])(
928+
_get_tools
929+
)
930+
mcp.custom_route(f"{secret_prefix}/api/settings/tools", methods=["POST"])(
931+
_save_tools
932+
)
933+
mcp.custom_route(f"{secret_prefix}/api/settings/restart", methods=["POST"])(
934+
_restart_addon
935+
)
936+
mcp.custom_route(f"{secret_prefix}/api/settings/info", methods=["GET"])(
937+
_settings_info
938+
)

tests/src/unit/test_settings_ui.py

Lines changed: 91 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -142,15 +142,88 @@ class TestConfigPath:
142142

143143
def test_addon_path_when_supervisor_token_set(self, monkeypatch):
144144
monkeypatch.setenv("SUPERVISOR_TOKEN", "fake")
145+
monkeypatch.delenv("HA_MCP_CONFIG_DIR", raising=False)
145146
assert _get_config_path() == Path("/data/tool_config.json")
146147

147148
def test_home_path_when_no_supervisor_token(self, monkeypatch, tmp_path):
148149
monkeypatch.delenv("SUPERVISOR_TOKEN", raising=False)
150+
monkeypatch.delenv("HA_MCP_CONFIG_DIR", raising=False)
149151
monkeypatch.setattr(Path, "home", lambda: tmp_path)
150152
result = _get_config_path()
151153
assert result == tmp_path / ".ha-mcp" / "tool_config.json"
152154
assert (tmp_path / ".ha-mcp").is_dir()
153155

156+
def test_honors_ha_mcp_config_dir_env_var(self, monkeypatch, tmp_path):
157+
"""HA_MCP_CONFIG_DIR overrides the default location.
158+
159+
Lets users on read-only Docker setups bind-mount a writable directory
160+
without relying on $HOME being set correctly. Takes precedence over
161+
SUPERVISOR_TOKEN so add-on users can override too.
162+
"""
163+
custom_dir = tmp_path / "custom"
164+
monkeypatch.setenv("HA_MCP_CONFIG_DIR", str(custom_dir))
165+
monkeypatch.setenv("SUPERVISOR_TOKEN", "fake") # should be ignored
166+
result = _get_config_path()
167+
assert result == custom_dir / "tool_config.json"
168+
assert custom_dir.is_dir()
169+
170+
def test_falls_back_to_tmpdir_when_home_unwritable(self, monkeypatch, tmp_path):
171+
"""When Path.home()/.ha-mcp can't be created (read-only fs / HOME=/),
172+
fall back to a tmp dir instead of crashing."""
173+
monkeypatch.delenv("SUPERVISOR_TOKEN", raising=False)
174+
monkeypatch.delenv("HA_MCP_CONFIG_DIR", raising=False)
175+
# Simulate Path.home() returning a path under a read-only filesystem.
176+
readonly_home = tmp_path / "readonly-home"
177+
readonly_home.mkdir()
178+
monkeypatch.setattr(Path, "home", lambda: readonly_home)
179+
# Force mkdir on the .ha-mcp dir to fail with a read-only OSError.
180+
original_mkdir = Path.mkdir
181+
182+
def fake_mkdir(self: Path, *args, **kwargs):
183+
if self == readonly_home / ".ha-mcp":
184+
raise OSError(30, "Read-only file system")
185+
return original_mkdir(self, *args, **kwargs)
186+
187+
monkeypatch.setattr(Path, "mkdir", fake_mkdir)
188+
# Direct the tmpdir fallback into tmp_path so we can assert location.
189+
fallback_root = tmp_path / "fallback-tmp"
190+
fallback_root.mkdir()
191+
monkeypatch.setattr(
192+
"ha_mcp.settings_ui.tempfile.gettempdir", lambda: str(fallback_root)
193+
)
194+
195+
result = _get_config_path()
196+
197+
assert result == fallback_root / "ha-mcp" / "tool_config.json"
198+
assert (fallback_root / "ha-mcp").is_dir()
199+
200+
def test_load_tool_config_does_not_crash_when_home_unwritable(
201+
self, monkeypatch, tmp_path
202+
):
203+
"""Regression for #1125: server startup must not raise when the
204+
home directory is read-only (e.g. Docker with read_only: true)."""
205+
monkeypatch.delenv("SUPERVISOR_TOKEN", raising=False)
206+
monkeypatch.delenv("HA_MCP_CONFIG_DIR", raising=False)
207+
readonly_home = tmp_path / "readonly-home"
208+
readonly_home.mkdir()
209+
monkeypatch.setattr(Path, "home", lambda: readonly_home)
210+
original_mkdir = Path.mkdir
211+
212+
def fake_mkdir(self: Path, *args, **kwargs):
213+
if self == readonly_home / ".ha-mcp":
214+
raise OSError(30, "Read-only file system")
215+
return original_mkdir(self, *args, **kwargs)
216+
217+
monkeypatch.setattr(Path, "mkdir", fake_mkdir)
218+
fallback_root = tmp_path / "fallback-tmp"
219+
fallback_root.mkdir()
220+
monkeypatch.setattr(
221+
"ha_mcp.settings_ui.tempfile.gettempdir", lambda: str(fallback_root)
222+
)
223+
224+
# Must not raise, must return a usable dict.
225+
assert load_tool_config() == {}
226+
154227

155228
class TestFeatureGatedTools:
156229
"""Test the FEATURE_GATED_TOOLS dict aligns with the beta tag system."""
@@ -168,7 +241,12 @@ def test_filesystem_tools_use_addon_option_name(self):
168241
# disabled_by should reference the dev addon option name (matches
169242
# how the JS renders "set <code>{disabled_by}</code> in the dev
170243
# add-on config or the matching env var (see docs/beta.md)").
171-
for name in ("ha_list_files", "ha_read_file", "ha_write_file", "ha_delete_file"):
244+
for name in (
245+
"ha_list_files",
246+
"ha_read_file",
247+
"ha_write_file",
248+
"ha_delete_file",
249+
):
172250
assert FEATURE_GATED_TOOLS[name]["disabled_by"] == "enable_filesystem_tools"
173251

174252

@@ -230,6 +308,7 @@ def decorator(fn):
230308
if path == "/api/settings/tools" and "POST" in methods:
231309
captured["save"] = fn
232310
return fn
311+
233312
return decorator
234313

235314
mcp = MagicMock()
@@ -276,13 +355,17 @@ async def test_drops_garbage_state_values(self, monkeypatch, tmp_path):
276355
config_path = tmp_path / "tool_config.json"
277356
monkeypatch.setattr("ha_mcp.settings_ui._get_config_path", lambda: config_path)
278357
save = self._capture_handler(monkeypatch)
279-
resp = await save(self._make_request({
280-
"states": {
281-
"ha_good_tool": "disabled",
282-
"ha_bad_value": "not_a_real_state",
283-
42: "disabled", # non-string key
284-
},
285-
}))
358+
resp = await save(
359+
self._make_request(
360+
{
361+
"states": {
362+
"ha_good_tool": "disabled",
363+
"ha_bad_value": "not_a_real_state",
364+
42: "disabled", # non-string key
365+
},
366+
}
367+
)
368+
)
286369
assert resp.status_code == 200
287370
saved = json.loads(config_path.read_text())
288371
assert saved["tools"] == {"ha_good_tool": "disabled"}

0 commit comments

Comments
 (0)