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).
This commit is contained in:
@@ -78,6 +78,6 @@ async def test_zigbee_connection(
|
|||||||
raise HTTPException(status_code=500, detail=str(exc)) from exc
|
raise HTTPException(status_code=500, detail=str(exc)) from exc
|
||||||
except (ConnectionError, TimeoutError) as exc:
|
except (ConnectionError, TimeoutError) as exc:
|
||||||
return ZigbeeTestConnectionResponse(connected=False, message=str(exc))
|
return ZigbeeTestConnectionResponse(connected=False, message=str(exc))
|
||||||
except Exception as exc:
|
except Exception:
|
||||||
logger.exception("Unexpected error during connection test")
|
logger.exception("Unexpected error during connection test")
|
||||||
return ZigbeeTestConnectionResponse(connected=False, message=f"Unexpected error: {exc}")
|
return ZigbeeTestConnectionResponse(connected=False, message="Unexpected error")
|
||||||
|
|||||||
@@ -21,6 +21,30 @@ _CONNECTION_TIMEOUT = 5.0 # seconds to verify broker reachability
|
|||||||
_NETWORKMAP_TIMEOUT = 10.0 # seconds to wait for the networkmap response
|
_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:
|
def _build_tls_context(insecure: bool) -> ssl.SSLContext:
|
||||||
"""Build an SSL context for MQTT TLS. If insecure, skip verification."""
|
"""Build an SSL context for MQTT TLS. If insecure, skip verification."""
|
||||||
ctx = ssl.create_default_context()
|
ctx = ssl.create_default_context()
|
||||||
@@ -231,11 +255,9 @@ async def fetch_networkmap(
|
|||||||
await asyncio.wait_for(_wait_for_response(), timeout=_NETWORKMAP_TIMEOUT)
|
await asyncio.wait_for(_wait_for_response(), timeout=_NETWORKMAP_TIMEOUT)
|
||||||
|
|
||||||
except aiomqtt.MqttError as exc:
|
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:
|
except asyncio.TimeoutError as exc:
|
||||||
raise TimeoutError(
|
raise TimeoutError("Timed out waiting for networkmap response") from exc
|
||||||
f"Timed out waiting for networkmap response from {mqtt_host}:{mqtt_port}"
|
|
||||||
) from exc
|
|
||||||
|
|
||||||
if not response_payload:
|
if not response_payload:
|
||||||
raise ValueError("Empty networkmap response received")
|
raise ValueError("Empty networkmap response received")
|
||||||
@@ -271,8 +293,6 @@ async def test_mqtt_connection(
|
|||||||
):
|
):
|
||||||
return True
|
return True
|
||||||
except aiomqtt.MqttError as exc:
|
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:
|
except asyncio.TimeoutError as exc:
|
||||||
raise TimeoutError(
|
raise TimeoutError("Connection to broker timed out") from exc
|
||||||
f"Connection to {mqtt_host}:{mqtt_port} timed out"
|
|
||||||
) from exc
|
|
||||||
|
|||||||
@@ -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"]
|
ctx = mock_aiomqtt.Client.call_args.kwargs["tls_context"]
|
||||||
assert ctx.verify_mode == ssl.CERT_NONE
|
assert ctx.verify_mode == ssl.CERT_NONE
|
||||||
assert ctx.check_hostname is False
|
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"
|
||||||
|
|||||||
Reference in New Issue
Block a user