Skip to content

Commit 8f8efc0

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 8f8efc0

7 files changed

Lines changed: 476 additions & 44 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: 48 additions & 13 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,36 @@ 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)
129143
return result
130-
except (OSError, json.JSONDecodeError):
131-
logger.warning("Failed to read tool config from %s", path)
144+
except json.JSONDecodeError:
145+
logger.warning("Tool config at %s is not valid JSON; ignoring.", path)
132146

133147
if settings is None:
134148
return {}
@@ -156,14 +170,23 @@ def load_tool_config(settings: Settings | None = None) -> dict[str, Any]:
156170
return {}
157171

158172

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

168191

169192
async def _get_tool_metadata(server: HomeAssistantSmartMCPServer) -> list[dict[str, Any]]:
@@ -760,7 +783,19 @@ async def _save_tools(request: Request) -> JSONResponse:
760783

761784
config = load_tool_config()
762785
config["tools"] = states
763-
save_tool_config(config)
786+
if not save_tool_config(config):
787+
return JSONResponse(
788+
create_error_response(
789+
ErrorCode.INTERNAL_ERROR,
790+
"Failed to persist tool config to disk",
791+
suggestions=[
792+
"Set HA_MCP_CONFIG_DIR to a writable path "
793+
"(common with read_only Docker setups)",
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: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
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 by
4+
both ``settings_ui`` (tool config) and ``usage_logger`` (rolling JSONL).
5+
"""
6+
7+
from __future__ import annotations
8+
9+
import logging
10+
import os
11+
import tempfile
12+
from pathlib import Path
13+
14+
logger = logging.getLogger(__name__)
15+
16+
# Cached resolved data directory. Populated on first ``get_data_dir()``
17+
# call so any warnings about unwritable dirs / fallback usage emit once
18+
# at startup rather than on every save/load HTTP request.
19+
_DATA_DIR_CACHE: Path | None = None
20+
21+
22+
def _is_addon() -> bool:
23+
"""Return True when running inside the Home Assistant add-on container.
24+
25+
Mirrors the convention in ``settings_ui.py`` of treating
26+
``SUPERVISOR_TOKEN`` as the add-on detector — more reliable than
27+
checking for ``/data`` because some Docker setups have a ``/data``
28+
directory that isn't the supervisor data dir.
29+
"""
30+
return bool(os.environ.get("SUPERVISOR_TOKEN"))
31+
32+
33+
def get_data_dir() -> Path:
34+
"""Return a writable directory for ha-mcp persistent data (memoized).
35+
36+
Priority:
37+
38+
1. ``HA_MCP_CONFIG_DIR`` env var — explicit override, e.g. for hardened
39+
Docker setups bind-mounting a writable volume into a
40+
``read_only: true`` container.
41+
2. ``/data`` — Home Assistant add-on (writable supervisor data dir).
42+
3. ``~/.ha-mcp`` — standard.
43+
4. ``<tempdir>/ha-mcp`` — last-resort fallback when (1) and (3) fail
44+
(read-only filesystem, or ``HOME`` unset so ``Path.home()`` resolves
45+
to ``/``). Loses persistence across restarts but lets the server
46+
start; users wanting persistence should set ``HA_MCP_CONFIG_DIR``.
47+
"""
48+
global _DATA_DIR_CACHE
49+
if _DATA_DIR_CACHE is None:
50+
_DATA_DIR_CACHE = _resolve_data_dir()
51+
return _DATA_DIR_CACHE
52+
53+
54+
def _resolve_data_dir() -> Path:
55+
"""Resolve the data directory; falls back if writes are blocked."""
56+
config_dir_env = os.environ.get("HA_MCP_CONFIG_DIR")
57+
preferred: Path | None = None
58+
if config_dir_env:
59+
custom_dir = Path(config_dir_env)
60+
try:
61+
custom_dir.mkdir(parents=True, exist_ok=True)
62+
return custom_dir
63+
except OSError as e:
64+
logger.warning(
65+
"HA_MCP_CONFIG_DIR=%s could not be prepared (%s: %s); "
66+
"falling back to a tmpdir.",
67+
custom_dir,
68+
type(e).__name__,
69+
e,
70+
)
71+
preferred = 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+
return home_dir
81+
except OSError:
82+
preferred = home_dir
83+
84+
fallback = Path(tempfile.gettempdir()) / "ha-mcp"
85+
try:
86+
fallback.mkdir(parents=True, exist_ok=True)
87+
logger.warning(
88+
"Cannot write ha-mcp data to %s (read-only filesystem or HOME unset). "
89+
"Falling back to %s — data will NOT persist across restarts. "
90+
"Set HA_MCP_CONFIG_DIR to a writable path for persistence.",
91+
preferred,
92+
fallback,
93+
)
94+
except OSError as e:
95+
# Even the tmpdir is unwritable. Return the path anyway so the
96+
# caller's own try/except (save_tool_config wraps writes in
97+
# try/except OSError; usage_logger disables itself) can degrade
98+
# gracefully.
99+
logger.warning(
100+
"Cannot write ha-mcp data to %s or fallback %s (%s: %s); "
101+
"persistence is disabled. "
102+
"Set HA_MCP_CONFIG_DIR to a writable path for persistence.",
103+
preferred,
104+
fallback,
105+
type(e).__name__,
106+
e,
107+
)
108+
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)