From 9970780e7af23f7e7b30ee54f7b4d656fe8a8eda Mon Sep 17 00:00:00 2001 From: Pouzor Date: Wed, 6 May 2026 16:50:01 +0200 Subject: [PATCH] fix(zigbee): sanitize MQTT error messages to prevent credential leakage aiomqtt/paho exception strings can include the broker URI with embedded credentials (mqtt://user:pass@host) or auth detail. The 502 response from /import and the message field on /test-connection echoed these verbatim via str(exc). - Add _sanitize_mqtt_error() that maps known patterns (auth, refused, DNS, TLS, timeout) to coarse, credential-free categories - Original exception still logged at WARNING level for operator debug - Drop hostname:port from TimeoutError messages - /test-connection unexpected-error path no longer interpolates exc Tests: 6 new (auth/refused/DNS/TLS/unknown sanitization, end-to-end fetch_networkmap leak check). --- backend/app/api/routes/zigbee.py | 4 +- backend/app/services/zigbee_service.py | 36 +++++++++++---- backend/tests/test_zigbee_service.py | 64 ++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 10 deletions(-) diff --git a/backend/app/api/routes/zigbee.py b/backend/app/api/routes/zigbee.py index c52488d..251678b 100644 --- a/backend/app/api/routes/zigbee.py +++ b/backend/app/api/routes/zigbee.py @@ -78,6 +78,6 @@ async def test_zigbee_connection( raise HTTPException(status_code=500, detail=str(exc)) from exc except (ConnectionError, TimeoutError) as exc: return ZigbeeTestConnectionResponse(connected=False, message=str(exc)) - except Exception as exc: + except Exception: logger.exception("Unexpected error during connection test") - return ZigbeeTestConnectionResponse(connected=False, message=f"Unexpected error: {exc}") + return ZigbeeTestConnectionResponse(connected=False, message="Unexpected error") diff --git a/backend/app/services/zigbee_service.py b/backend/app/services/zigbee_service.py index b4234f4..ee57e58 100644 --- a/backend/app/services/zigbee_service.py +++ b/backend/app/services/zigbee_service.py @@ -21,6 +21,30 @@ _CONNECTION_TIMEOUT = 5.0 # seconds to verify broker reachability _NETWORKMAP_TIMEOUT = 10.0 # seconds to wait for the networkmap response +def _sanitize_mqtt_error(exc: BaseException) -> str: + """Return a generic, credential-free message for an MQTT error. + + The raw aiomqtt/paho error string can include the broker URI with + embedded credentials (e.g. ``mqtt://user:pass@host``) or auth-related + detail that should not leak to API clients. Map known patterns to + coarse categories; default to a generic failure message. The original + exception is logged at WARNING level for operator debugging. + """ + logger.warning("MQTT error (sanitized for client): %r", exc) + raw = str(exc).lower() + if "not authoriz" in raw or "bad user" in raw or "bad username" in raw: + return "Authentication failed" + if "refused" in raw: + return "Connection refused by broker" + if "name or service not known" in raw or "getaddrinfo" in raw or "nodename nor servname" in raw: + return "Broker hostname could not be resolved" + if "ssl" in raw or "tls" in raw or "certificate" in raw: + return "TLS handshake failed" + if "timed out" in raw or "timeout" in raw: + return "Connection to broker timed out" + return "MQTT connection failed" + + def _build_tls_context(insecure: bool) -> ssl.SSLContext: """Build an SSL context for MQTT TLS. If insecure, skip verification.""" ctx = ssl.create_default_context() @@ -231,11 +255,9 @@ async def fetch_networkmap( await asyncio.wait_for(_wait_for_response(), timeout=_NETWORKMAP_TIMEOUT) except aiomqtt.MqttError as exc: - raise ConnectionError(f"MQTT connection failed: {exc}") from exc + raise ConnectionError(_sanitize_mqtt_error(exc)) from exc except asyncio.TimeoutError as exc: - raise TimeoutError( - f"Timed out waiting for networkmap response from {mqtt_host}:{mqtt_port}" - ) from exc + raise TimeoutError("Timed out waiting for networkmap response") from exc if not response_payload: raise ValueError("Empty networkmap response received") @@ -271,8 +293,6 @@ async def test_mqtt_connection( ): return True except aiomqtt.MqttError as exc: - raise ConnectionError(f"MQTT connection failed: {exc}") from exc + raise ConnectionError(_sanitize_mqtt_error(exc)) from exc except asyncio.TimeoutError as exc: - raise TimeoutError( - f"Connection to {mqtt_host}:{mqtt_port} timed out" - ) from exc + raise TimeoutError("Connection to broker timed out") from exc diff --git a/backend/tests/test_zigbee_service.py b/backend/tests/test_zigbee_service.py index 3bf4e5c..bc76d84 100644 --- a/backend/tests/test_zigbee_service.py +++ b/backend/tests/test_zigbee_service.py @@ -437,3 +437,67 @@ async def test_test_mqtt_connection_insecure_passes_no_verify_context() -> None: ctx = mock_aiomqtt.Client.call_args.kwargs["tls_context"] assert ctx.verify_mode == ssl.CERT_NONE assert ctx.check_hostname is False + + +# --------------------------------------------------------------------------- +# Sanitize MQTT errors +# --------------------------------------------------------------------------- + +from app.services.zigbee_service import _sanitize_mqtt_error # noqa: E402 + + +def test_sanitize_auth_error_does_not_leak_credentials() -> None: + msg = _sanitize_mqtt_error( + Exception("Not authorized: bad username or password for user=admin pwd=secret") + ) + assert msg == "Authentication failed" + assert "admin" not in msg + assert "secret" not in msg + + +def test_sanitize_refused() -> None: + assert _sanitize_mqtt_error(Exception("Connection refused")) == "Connection refused by broker" + + +def test_sanitize_dns_failure_strips_host() -> None: + msg = _sanitize_mqtt_error( + Exception("[Errno 8] nodename nor servname provided, or not known: broker.internal.lan") + ) + assert msg == "Broker hostname could not be resolved" + assert "broker.internal.lan" not in msg + + +def test_sanitize_tls_error() -> None: + assert _sanitize_mqtt_error( + Exception("[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed") + ) == "TLS handshake failed" + + +def test_sanitize_unknown_falls_back_to_generic() -> None: + msg = _sanitize_mqtt_error(Exception("mqtt://admin:hunter2@broker:1883 weird state")) + assert msg == "MQTT connection failed" + assert "hunter2" not in msg + assert "admin" not in msg + + +@pytest.mark.asyncio +async def test_fetch_networkmap_does_not_leak_creds_in_connection_error() -> None: + class _FakeClient: + async def __aenter__(self): + raise Exception("Not authorized: rejected mqtt://admin:hunter2@host") + + async def __aexit__(self, *_): + pass + + with patch("app.services.zigbee_service.aiomqtt") as mock_aiomqtt: + mock_aiomqtt.Client.return_value = _FakeClient() + mock_aiomqtt.MqttError = Exception + + with pytest.raises(ConnectionError) as ei: + await fetch_networkmap( + mqtt_host="host", mqtt_port=1883, base_topic="zigbee2mqtt" + ) + msg = str(ei.value) + assert "hunter2" not in msg + assert "admin" not in msg + assert msg == "Authentication failed"