Skip to content

Commit 254c515

Browse files
fix: reject dangling registry references across all registry write tools (#2162)
* fix: generalize registry-reference validation to every registry kind `validate_registry_ids` only covered area_id, labels, and a single "helpers"-scoped category, and failed open on every lookup but the area one. Issue #2159 reproduced dangling references on the other surfaces, so the validator now takes a scope->category_id mapping plus floor_id, and a single `fail_closed` flag applies to every requested lookup: a registry that cannot be read rejects the write with CONNECTION_FAILED instead of waving the reference through. The two helper call sites move to the new signature and opt into fail-closed — a degraded lookup must never allow a dangling reference. Per-registry checks are extracted so the entry point stays well inside the C901 limit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: reject dangling registry references across registry write tools Issue #2159: every remaining write tool that takes a cross-registry reference forwarded it to Home Assistant unchecked, and HA stores an unknown ID verbatim, so the tool reported success while leaving the registry pointing at something that does not exist. - ha_set_entity: one preflight at the tool entry now covers area_id, labels, and categories for both the single and bulk paths, replacing the per-entity area check that bulk would have multiplied by N. Labels are only validated for label_operation set/add — removing an unknown label is the cleanup path for these dangling references and must stay possible. - ha_set_device: area_id and the replacement label set are validated before config/device_registry/update. - ha_set_area_or_floor: floor_id is validated before the area create or update. - ha_config_set_automation / ha_config_set_script: the category is applied post-upsert by apply_entity_category, so both category sources (the parameter and the config dict) are validated at tool entry — nothing is created when the category does not exist. - ha_config_set_scene: its private category validator now delegates to the shared one so all four tools reject dangling references the same way. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: cover registry-reference validation on every write surface Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: validate single-entity refs at the write site; honor script transform category Round 1 CI + bot review feedback: - Single-entity validation moved into _update_single_entity immediately before the registry write (restores the #2160 check-to-write window, flagged by Codex); bulk keeps one entry preflight via preflighted=True. - ha_config_set_script python_transform now honors category: popped from the transformed config, validated fail-closed, applied post-upsert (parity with automations, flagged by CodeRabbit). - Tests adapted: e2e device-labels test now creates its labels first; helper fail-open test flipped to a type-keyed mock; bulk/add mock chains reordered for the new call order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore: ruff format Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: restore original device labels in e2e cleanup Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: address second-round review — category recheck, script rename resolution, edge hardening - apply_entity_category rechecks the scope registry immediately before the write; a category deleted during the upsert/wait gap degrades to a warning instead of a dangling reference (Codex). - ha_config_set_script resolves the storage key to the current entity_id before category application and registration wait — a registry-renamed script no longer sends the category to a nonexistent entity (Codex). - Empty-string elements inside a labels list are rejected as unknown IDs instead of riding the write (Codex). - Fail-closed lookup errors preserve their classification: auth failures surface as AUTH_*, not CONNECTION_FAILED (Codex). - e2e device-labels test creates its labels inside the cleanup guard so a failed second create cannot leak the first (Codex). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: classify auth failures on both WS channels; pin flow-helper fail-closed and scan fallback; gate script resolution Maintainer review (three concerns): - The connection manager now carries connect()'s exception object and re-raises an auth failure as HomeAssistantAuthError instead of collapsing it into the connection error; the failure-envelope channel discriminates on HA's preserved error_code (unauthorized -> AUTH_*), mirroring _resolve_device_id_for_entity. Both shapes pinned. - Pinned the previously untested behaviors: fail_closed=True at the _handle_flow_helper call site and the registry-list rename fallback in _resolve_script_entity_id (component-less installs). - _commit_script_config resolves the entity id only when wait or a category consumes it; the wait=False bulk path skips the lookups. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: pin zero WebSocket calls on the ungated script write path Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: classify acquisition-phase auth failures via the preserved cause chain The transport wrap re-raises acquisition failures as HomeAssistantConnectionError 'from e' (phase-before-type, deliberately pinned), so the manager's auth raise never reached the caller as an auth class. The fail-closed branch now walks the __cause__ chain — mirroring its error_code discrimination — and the pin runs through the REAL send_websocket_message as the review required. The mocked direct-raise test is replaced with the wrapped-cause shape the client actually produces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: isolate the real-transport auth test from global settings Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 parent 956d8a9 commit 254c515

22 files changed

Lines changed: 1853 additions & 284 deletions

src/ha_mcp/client/websocket_client.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,12 @@ def __init__(self, url: str, token: str, verify_ssl: bool | None = None):
292292
# or None. Surfaced by callers so the agent sees *why* a WebSocket
293293
# connection failed instead of an opaque "Failed to connect" string.
294294
self._last_connect_error: str | None = None
295+
# The exception object itself, so the connection manager can
296+
# re-raise an auth failure AS an auth failure instead of collapsing
297+
# every connect miss into HomeAssistantConnectionError (issue #2159
298+
# review: an expired token must classify as AUTH_*, and the reason
299+
# otherwise survives only inside the message string).
300+
self._last_connect_exception: Exception | None = None
295301

296302
# Parse URL to get WebSocket endpoint
297303
parsed = urlparse(self.base_url)
@@ -317,6 +323,7 @@ async def connect(self) -> bool:
317323
logger.info(f"Connecting to Home Assistant WebSocket: {self.ws_url}")
318324
self._state.reset_connection()
319325
self._last_connect_error = None
326+
self._last_connect_exception = None
320327

321328
# Only configure an SSLContext for wss://; ws:// (Supervisor
322329
# proxy) doesn't use TLS and gets ssl=None.
@@ -383,6 +390,7 @@ async def connect(self) -> bool:
383390

384391
except Exception as e:
385392
self._last_connect_error = f"{type(e).__name__}: {e}"
393+
self._last_connect_exception = e
386394
if _is_ssl_error(e) and self.verify_ssl:
387395
logger.error(
388396
"WebSocket TLS verification failed for %s: %s. "
@@ -1037,6 +1045,16 @@ def last_connect_error(self) -> str | None:
10371045
"""
10381046
return self._last_connect_error
10391047

1048+
@property
1049+
def last_connect_exception(self) -> Exception | None:
1050+
"""The exception the most recent ``connect()`` attempt failed with.
1051+
1052+
Lets the connection manager preserve the failure class — an
1053+
``HomeAssistantAuthError`` re-raises as an auth error rather than
1054+
being collapsed into ``HomeAssistantConnectionError``.
1055+
"""
1056+
return self._last_connect_exception
1057+
10401058

10411059
MAX_POOL_SIZE = 50
10421060

@@ -1295,6 +1313,13 @@ async def get_client(
12951313
# keeps a non-str (e.g. a MagicMock in tests) from polluting
12961314
# the message with a repr.
12971315
detail = f": {reason}" if isinstance(reason, str) else ""
1316+
# An auth failure must classify as an auth failure — the
1317+
# collapsed connection error buries the cause in the message
1318+
# string and callers misreport it as connection guidance.
1319+
if isinstance(client.last_connect_exception, HomeAssistantAuthError):
1320+
raise HomeAssistantAuthError(
1321+
"WebSocket authentication failed" + detail
1322+
)
12981323
raise HomeAssistantConnectionError(
12991324
"Failed to connect to Home Assistant WebSocket" + detail
13001325
)

src/ha_mcp/tools/tools_areas.py

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
register_tool_methods,
2424
validate_identifier_not_empty,
2525
)
26+
from .tools_config_helpers import validate_registry_ids
2627
from .util_helpers import (
2728
JSON_STRING_COERCION,
2829
parse_string_list_param,
@@ -675,6 +676,14 @@ async def ha_set_area_or_floor(
675676
)
676677
)
677678

679+
# Issue #2159: the area registry stores an unknown floor_id
680+
# verbatim, orphaning the area. ``_validate_cross_kind_params``
681+
# already rejected floor_id for kind='floor', so this only ever
682+
# runs for areas; None and "" (clear) skip the lookup.
683+
await validate_registry_ids(
684+
self._client, None, None, None, floor_id=floor_id, fail_closed=True
685+
)
686+
678687
result = await self._client.send_websocket_message(message)
679688

680689
if result.get("success"):

src/ha_mcp/tools/tools_config_automations.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@
4747
validate_identifier_not_empty,
4848
)
4949
from .reference_validator import validate_config_references
50+
from .tools_config_helpers import validate_registry_ids
5051
from .util_helpers import (
5152
JSON_STRING_COERCION,
5253
apply_entity_category,
@@ -852,6 +853,17 @@ async def ha_config_set_automation(
852853
self._client, config_dict
853854
)
854855

856+
# Issue #2159: the category is applied post-upsert via
857+
# ``apply_entity_category``, which HA accepts unchecked. Reject an
858+
# unknown one here so no automation is created under it.
859+
await validate_registry_ids(
860+
self._client,
861+
None,
862+
None,
863+
{"automation": effective_category},
864+
fail_closed=True,
865+
)
866+
855867
return await self._run_config_update(
856868
config_dict,
857869
identifier,
@@ -1017,6 +1029,16 @@ async def _run_python_transform(
10171029
self._validate_required_fields(transformed_config, identifier)
10181030
bp_warnings = _check_best_practices(transformed_config)
10191031

1032+
# Issue #2159: reject an unknown category before the write, so a
1033+
# transform never lands under a category that does not exist.
1034+
await validate_registry_ids(
1035+
self._client,
1036+
None,
1037+
None,
1038+
{"automation": effective_category},
1039+
fail_closed=True,
1040+
)
1041+
10201042
# ``_fetch_and_verify_hash`` already resolved ``identifier`` to the
10211043
# storage key; thread it so the upsert skips the redundant re-resolve
10221044
# (issue #1813 Phase 0). Fall back to the raw identifier if the fetched

0 commit comments

Comments
 (0)