Skip to content

Commit 3a53371

Browse files
amccats3409claude
andcommitted
fix: survive read-only filesystems at startup (#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. Coordinated changes: **Shared resolver** (`utils/data_paths.py`, new): single source of truth for "where does ha-mcp write its persistent files". Priority: 1. `HA_MCP_CONFIG_DIR` env var — explicit override for hardened Docker setups bind-mounting a writable volume. 2. `/data` — Home Assistant add-on supervisor data dir. 3. `~/.ha-mcp` — standard. 4. `<tempdir>/ha-mcp` — last-resort fallback. The result is memoized so the fallback warning emits once at startup instead of on every save/load HTTP request. Replaces the previous inconsistent ad-hoc resolution in two places (`settings_ui` and `usage_logger`). **`settings_ui`**: `_get_config_path` becomes a thin `get_data_dir() / "tool_config.json"` wrapper. `load_tool_config` replaces the `path.exists()` + `read_text()` two-step with a single `try: read_text() except OSError:` — `Path.exists()` only swallows ENOENT/ENOTDIR/EBADF/ELOOP, so EACCES (e.g. `HA_MCP_CONFIG_DIR` pointing at an existing 0700 dir owned by another UID) would re-introduce the same class of crash. `save_tool_config` returns `bool` so the `_save_tools` HTTP route can return 500 when the write fails — the JS already handles `!resp.ok` with "Save failed!", but it was never given the chance. **`usage_logger`**: switch the default log path from `Path.home() / ".ha-mcp" / "logs"` to `get_data_dir() / "logs"`. Logs now follow the same precedence as the settings-UI tool config — honors `HA_MCP_CONFIG_DIR` so users with a bind-mount don't lose their logs, lands in `/data/logs/` in add-on mode (the supervisor's persistent location, was previously going to ephemeral `~/.ha-mcp/logs`). **Dockerfile**: set `ENV HOME=/home/mcpuser` so `Path.home()` resolves correctly under the default user. `chmod 0755 /home/mcpuser` because the base image's `/etc/login.defs` sets `HOME_MODE=0700`; that's fine when the container runs as mcpuser, but un-traversable when users override `--user UID:GID` (the issue reporter does so) — anything that stats a path under HOME then raises PermissionError. Mode 0755 keeps write access restricted to mcpuser. Tests: 9 new in `test_data_paths.py` and `test_settings_ui.py` covering priority order, both fallback chains, memoization, the `load_tool_config` EACCES regression guard, the `save_tool_config` bool contract, and the 500-on-save-failure HTTP path. Plus a `usage_logger` test verifying it uses the shared resolver. 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 3a53371

7 files changed

Lines changed: 489 additions & 45 deletions

File tree

Dockerfile

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,14 @@ 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. The base image's HOME_MODE is 0700 — fine when running
35+
# as mcpuser, but it makes /home/mcpuser un-traversable when users override
36+
# `--user UID:GID` (the issue #1125 reporter does so). Any code that stats a
37+
# path under HOME (set below) then raises PermissionError. chmod 0755 makes
38+
# the dir traversable for any uid; write access stays restricted to mcpuser.
39+
RUN groupadd -r mcpuser \
40+
&& useradd -r -g mcpuser -m mcpuser \
41+
&& chmod 0755 /home/mcpuser
3642

3743
WORKDIR /app
3844

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

4450
USER mcpuser
4551

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

src/ha_mcp/settings_ui.py

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
from .errors import ErrorCode, create_error_response
2323
from .transforms import DEFAULT_PINNED_TOOLS
24+
from .utils.data_paths import get_data_dir
2425

2526
if TYPE_CHECKING:
2627
from fastmcp import FastMCP
@@ -112,23 +113,37 @@ def _is_addon() -> bool:
112113

113114

114115
def _get_config_path() -> Path:
115-
"""Return the path to the tool config JSON file."""
116-
if _is_addon():
117-
return Path("/data") / "tool_config.json"
118-
home_dir = Path.home() / ".ha-mcp"
119-
home_dir.mkdir(parents=True, exist_ok=True)
120-
return home_dir / "tool_config.json"
116+
"""Return the path to the tool config JSON file.
117+
118+
Delegates directory resolution to :func:`utils.data_paths.get_data_dir`,
119+
which handles ``HA_MCP_CONFIG_DIR`` override, add-on ``/data``,
120+
home-dir, and tmpdir fallback (memoized).
121+
"""
122+
return get_data_dir() / "tool_config.json"
121123

122124

123125
def load_tool_config(settings: Settings | None = None) -> dict[str, Any]:
124126
"""Load persisted tool config, seeding from env vars if no file exists."""
125127
path = _get_config_path()
126-
if path.exists():
128+
# ``Path.exists()`` only swallows ``ENOENT/ENOTDIR/EBADF/ELOOP``; an
129+
# ``EACCES`` (e.g. ``HA_MCP_CONFIG_DIR`` pointing at a dir that exists
130+
# but isn't readable by the runtime UID) propagates. Read directly and
131+
# treat ``FileNotFoundError`` as "no config yet"; log other ``OSError``s.
132+
try:
133+
raw = path.read_text()
134+
except FileNotFoundError:
135+
raw = None
136+
except OSError:
137+
logger.warning("Cannot read tool config at %s", path)
138+
raw = None
139+
140+
if raw is not None:
127141
try:
128-
result: dict[str, Any] = json.loads(path.read_text())
142+
result: dict[str, Any] = json.loads(raw)
143+
except json.JSONDecodeError:
144+
logger.warning("Tool config at %s is not valid JSON; ignoring.", path)
145+
else:
129146
return result
130-
except (OSError, json.JSONDecodeError):
131-
logger.warning("Failed to read tool config from %s", path)
132147

133148
if settings is None:
134149
return {}
@@ -156,14 +171,23 @@ def load_tool_config(settings: Settings | None = None) -> dict[str, Any]:
156171
return {}
157172

158173

159-
def save_tool_config(config: dict[str, Any]) -> None:
160-
"""Persist tool config to disk."""
174+
def save_tool_config(config: dict[str, Any]) -> bool:
175+
"""Persist tool config to disk.
176+
177+
Returns True on success, False on failure (read-only filesystem,
178+
permission denied, etc.). Caller is responsible for surfacing the
179+
failure to the user — the HTTP route at ``_save_tools`` returns 500
180+
so the UI's ``saveConfig`` shows "Save failed!" instead of the
181+
misleading "Saved — restart required".
182+
"""
161183
path = _get_config_path()
162184
try:
163185
path.write_text(json.dumps(config, indent=2))
164-
logger.info("Saved tool config to %s", path)
165186
except OSError:
166187
logger.exception("Failed to save tool config to %s", path)
188+
return False
189+
logger.info("Saved tool config to %s", path)
190+
return True
167191

168192

169193
async def _get_tool_metadata(server: HomeAssistantSmartMCPServer) -> list[dict[str, Any]]:
@@ -760,7 +784,18 @@ async def _save_tools(request: Request) -> JSONResponse:
760784

761785
config = load_tool_config()
762786
config["tools"] = states
763-
save_tool_config(config)
787+
if not save_tool_config(config):
788+
return JSONResponse(
789+
create_error_response(
790+
ErrorCode.INTERNAL_ERROR,
791+
"Failed to persist tool config to disk",
792+
suggestions=[
793+
"Set HA_MCP_CONFIG_DIR to a writable path (read-only filesystem?)",
794+
"Check the server logs for the underlying OSError",
795+
],
796+
),
797+
status_code=500,
798+
)
764799

765800
disabled_count = sum(1 for s in states.values() if s == "disabled")
766801
pinned_count = sum(1 for s in states.values() if s == "pinned")

src/ha_mcp/utils/data_paths.py

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
"""Resolve a writable directory for ha-mcp persistent data.
2+
3+
Single source of truth for "where does ha-mcp write its files?" — used
4+
by both ``settings_ui`` (tool config) and ``usage_logger`` (rolling
5+
JSONL).
6+
"""
7+
8+
from __future__ import annotations
9+
10+
import functools
11+
import logging
12+
import os
13+
import tempfile
14+
from pathlib import Path
15+
16+
logger = logging.getLogger(__name__)
17+
18+
19+
def _is_addon() -> bool:
20+
"""Return True when running inside the Home Assistant add-on container.
21+
22+
Mirrors the convention in ``settings_ui.py`` of treating
23+
``SUPERVISOR_TOKEN`` as the add-on detector — more reliable than
24+
checking for ``/data`` because some Docker setups have a ``/data``
25+
directory that isn't the supervisor data dir.
26+
"""
27+
return bool(os.environ.get("SUPERVISOR_TOKEN"))
28+
29+
30+
@functools.lru_cache(maxsize=1)
31+
def get_data_dir() -> Path:
32+
"""Return a writable directory for ha-mcp persistent data (memoized).
33+
34+
Priority:
35+
36+
1. ``HA_MCP_CONFIG_DIR`` env var — explicit override, e.g. for hardened
37+
Docker setups bind-mounting a writable volume into a
38+
``read_only: true`` container.
39+
2. ``/data`` — Home Assistant add-on (writable supervisor data dir).
40+
3. ``~/.ha-mcp`` — standard.
41+
4. ``<tempdir>/ha-mcp`` — last-resort fallback when (1) and (3) fail
42+
(read-only filesystem, or ``HOME`` unset so ``Path.home()`` resolves
43+
to ``/``). Loses persistence across restarts but lets the server
44+
start; users wanting persistence should set ``HA_MCP_CONFIG_DIR``.
45+
46+
Memoized so the fallback warning emits once at startup rather than on
47+
every save/load HTTP request. Tests reset via
48+
``get_data_dir.cache_clear()``.
49+
"""
50+
return _resolve_data_dir()
51+
52+
53+
def _resolve_data_dir() -> Path:
54+
"""Resolve the data directory (uncached); see ``get_data_dir`` for priority."""
55+
config_dir_env = os.environ.get("HA_MCP_CONFIG_DIR")
56+
preferred: Path | None = None
57+
if config_dir_env:
58+
custom_dir = Path(config_dir_env)
59+
try:
60+
custom_dir.mkdir(parents=True, exist_ok=True)
61+
except OSError as e:
62+
logger.warning(
63+
"HA_MCP_CONFIG_DIR=%s could not be prepared (%s: %s); "
64+
"falling back to a tmpdir.",
65+
custom_dir,
66+
type(e).__name__,
67+
e,
68+
)
69+
preferred = custom_dir
70+
else:
71+
return custom_dir
72+
73+
if _is_addon():
74+
return Path("/data")
75+
76+
if preferred is None:
77+
home_dir = Path.home() / ".ha-mcp"
78+
try:
79+
home_dir.mkdir(parents=True, exist_ok=True)
80+
except OSError:
81+
preferred = home_dir
82+
else:
83+
return home_dir
84+
85+
fallback = Path(tempfile.gettempdir()) / "ha-mcp"
86+
try:
87+
fallback.mkdir(parents=True, exist_ok=True)
88+
except OSError as e:
89+
# Even the tmpdir is unwritable. Return the path anyway so the
90+
# caller's own try/except (save_tool_config wraps writes in
91+
# try/except OSError; usage_logger disables itself) can degrade
92+
# gracefully.
93+
logger.warning(
94+
"Cannot write ha-mcp data to %s or fallback %s (%s: %s); "
95+
"persistence is disabled. "
96+
"Set HA_MCP_CONFIG_DIR to a writable path for persistence.",
97+
preferred,
98+
fallback,
99+
type(e).__name__,
100+
e,
101+
)
102+
else:
103+
logger.warning(
104+
"Cannot write ha-mcp data to %s (read-only filesystem or HOME unset). "
105+
"Falling back to %s — data will NOT persist across restarts. "
106+
"Set HA_MCP_CONFIG_DIR to a writable path for persistence.",
107+
preferred,
108+
fallback,
109+
)
110+
return fallback

src/ha_mcp/utils/usage_logger.py

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
from queue import Queue
1414
from typing import Any
1515

16+
from .data_paths import get_data_dir
17+
1618
# Default ring buffer size - keeps last N entries in memory
1719
DEFAULT_RING_BUFFER_SIZE = 200
1820

@@ -46,13 +48,15 @@ def emit(self, record: logging.LogRecord) -> None:
4648
return
4749

4850
with self._lock:
49-
self._logs.append({
50-
"timestamp": datetime.now(UTC).isoformat(),
51-
"level": record.levelname,
52-
"logger": record.name,
53-
"message": record.getMessage(),
54-
"elapsed_seconds": round(elapsed, 2),
55-
})
51+
self._logs.append(
52+
{
53+
"timestamp": datetime.now(UTC).isoformat(),
54+
"level": record.levelname,
55+
"logger": record.name,
56+
"message": record.getMessage(),
57+
"elapsed_seconds": round(elapsed, 2),
58+
}
59+
)
5660

5761
def get_logs(self) -> list[dict[str, Any]]:
5862
"""Get collected startup logs."""
@@ -115,9 +119,12 @@ def __init__(
115119
if log_file_path:
116120
self.log_file_path = Path(log_file_path)
117121
else:
118-
# Use user's home directory by default to avoid read-only filesystem errors
119-
# when running via uvx/npx which might have read-only CWD
120-
self.log_file_path = Path.home() / ".ha-mcp" / "logs" / "mcp_usage.jsonl"
122+
# Defer to the shared resolver so logs follow the same precedence
123+
# as the settings-UI tool config (HA_MCP_CONFIG_DIR > /data >
124+
# ~/.ha-mcp > tempdir). Avoids polluting the filesystem root when
125+
# HOME is unset and avoids surprising users who bind-mount a
126+
# writable volume via HA_MCP_CONFIG_DIR but find logs missing.
127+
self.log_file_path = get_data_dir() / "logs" / "mcp_usage.jsonl"
121128

122129
try:
123130
self.log_file_path.parent.mkdir(parents=True, exist_ok=True)

0 commit comments

Comments
 (0)