From 9a39c5b76a897e1df30b988e442f255324ebd182 Mon Sep 17 00:00:00 2001 From: Otto Winter Date: Thu, 26 Apr 2018 17:25:04 +0200 Subject: [PATCH 1/3] Improve MQTT topic validation --- homeassistant/components/mqtt/__init__.py | 52 +++++++++++++++++------ tests/components/mqtt/test_init.py | 52 +++++++++++++++++++++-- 2 files changed, 89 insertions(+), 15 deletions(-) diff --git a/homeassistant/components/mqtt/__init__.py b/homeassistant/components/mqtt/__init__.py index b81a4fc16a7143..1968865eb0ab99 100644 --- a/homeassistant/components/mqtt/__init__.py +++ b/homeassistant/components/mqtt/__init__.py @@ -90,22 +90,48 @@ MAX_RECONNECT_WAIT = 300 # seconds -def valid_subscribe_topic(value: Any, invalid_chars='\0') -> str: - """Validate that we can subscribe using this MQTT topic.""" +def valid_topic(value: Any) -> str: + """Validate that this is a valid topic name/filter.""" value = cv.string(value) - if all(c not in value for c in invalid_chars): - return vol.Length(min=1, max=65535)(value) - raise vol.Invalid('Invalid MQTT topic name') + try: + raw_value = value.encode('utf-8') + except UnicodeError: + raise vol.Invalid("MQTT topic name/filter must be valid UTF-8 string.") + vol.Length(min=1, max=65535)(raw_value) + if '\0' in value: + raise vol.Invalid("MQTT topic name/filter must not contain null " + "character.") + return value -def valid_publish_topic(value: Any) -> str: - """Validate that we can publish using this MQTT topic.""" - return valid_subscribe_topic(value, invalid_chars='#+\0') +def valid_subscribe_topic(value: Any) -> str: + """Validate that we can subscribe using this MQTT topic.""" + value = valid_topic(value) + for i in (i for i, c in enumerate(value) if c == '+'): + if (i > 0 and value[i - 1] != '/') or \ + (i < len(value) - 1 and value[i + 1] != '/'): + raise vol.Invalid("Single-level wildcard must occupy an entire " + "level of the filter") + + index = value.find('#') + if index != -1: + if index != len(value) - 1: + # If there are multiple wildcards, this will also trigger + raise vol.Invalid("Multi-level wildcard must be the last " + "character in the topic filter.") + if len(value) > 1 and value[index - 1] != '/': + raise vol.Invalid("Multi-level wildcard must be after a topic " + "level separator.") + + return value -def valid_discovery_topic(value: Any) -> str: - """Validate a discovery topic.""" - return valid_subscribe_topic(value, invalid_chars='#+\0/') +def valid_publish_topic(value: Any) -> str: + """Validate that we can publish using this MQTT topic.""" + value = valid_topic(value) + if '+' in value or '#' in value: + raise vol.Invalid("Wildcards can not be used in topic names") + return value _VALID_QOS_SCHEMA = vol.All(vol.Coerce(int), vol.In([0, 1, 2])) @@ -143,8 +169,10 @@ def valid_discovery_topic(value: Any) -> str: vol.Optional(CONF_WILL_MESSAGE): MQTT_WILL_BIRTH_SCHEMA, vol.Optional(CONF_BIRTH_MESSAGE): MQTT_WILL_BIRTH_SCHEMA, vol.Optional(CONF_DISCOVERY, default=DEFAULT_DISCOVERY): cv.boolean, + # discovery_prefix must be a valid publish topic because if no + # state topic is specified, it will be created with the given prefix. vol.Optional(CONF_DISCOVERY_PREFIX, - default=DEFAULT_DISCOVERY_PREFIX): valid_discovery_topic, + default=DEFAULT_DISCOVERY_PREFIX): valid_publish_topic, }), }, extra=vol.ALLOW_EXTRA) diff --git a/tests/components/mqtt/test_init.py b/tests/components/mqtt/test_init.py index b25479bb75acdf..5fe633e1964222 100644 --- a/tests/components/mqtt/test_init.py +++ b/tests/components/mqtt/test_init.py @@ -131,10 +131,56 @@ def test_service_call_with_ascii_qos_retain_flags(self): self.hass.data['mqtt'].async_publish.call_args[0][2], 2) self.assertFalse(self.hass.data['mqtt'].async_publish.call_args[0][3]) - def test_invalid_mqtt_topics(self): - """Test invalid topics.""" + def test_validate_topic(self): + """Test topic name/filter validation.""" + # Invalid UTF-8, must not contain U+D800 to U+DFFF. + self.assertRaises(vol.Invalid, mqtt.valid_topic, '\ud800') + self.assertRaises(vol.Invalid, mqtt.valid_topic, '\udfff') + # Topic MUST NOT be empty + self.assertRaises(vol.Invalid, mqtt.valid_topic, '') + # Topic MUST NOT be longer than 65535 encoded bytes. + self.assertRaises(vol.Invalid, mqtt.valid_topic, 'ΓΌ' * 32768) + # UTF-8 MUST NOT include null character + self.assertRaises(vol.Invalid, mqtt.valid_topic, 'bad\0one') + + # Topics "SHOULD NOT" include these special characters + # (not MUST NOT, RFC2119). The receiver MAY close the connection. + mqtt.valid_topic('\u0001') + mqtt.valid_topic('\u001F') + mqtt.valid_topic('\u009F') + mqtt.valid_topic('\u009F') + mqtt.valid_topic('\uffff') + + def test_validate_subscribe_topic(self): + """Test invalid subscribe topics.""" + mqtt.valid_subscribe_topic('#') + mqtt.valid_subscribe_topic('sport/#') + mqtt.valid_subscribe_topic('sport/#/') + self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'foo/bar#') + self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'foo/#/bar') + + mqtt.valid_subscribe_topic('+') + mqtt.valid_subscribe_topic('+/tennis/#') + self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'sport+') + self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'sport+/') + self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'sport/+1') + self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'sport/+#') + self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'bad+topic') + mqtt.valid_subscribe_topic('sport/+/player1') + mqtt.valid_subscribe_topic('/finance') + mqtt.valid_subscribe_topic('+/+') + mqtt.valid_subscribe_topic('$SYS/#') + + def test_validate_publish_topic(self): + """Test invalid publish topics.""" + self.assertRaises(vol.Invalid, mqtt.valid_publish_topic, 'pub+') + self.assertRaises(vol.Invalid, mqtt.valid_publish_topic, 'pub/+') + self.assertRaises(vol.Invalid, mqtt.valid_publish_topic, '1#') self.assertRaises(vol.Invalid, mqtt.valid_publish_topic, 'bad+topic') - self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'bad\0one') + mqtt.valid_publish_topic('//') + + # Topic names beginning with $ SHOULD NOT be used, but can + mqtt.valid_publish_topic('$SYS/') # pylint: disable=invalid-name From 08e487508a23f5e3dafba9e52a518a544ded3f04 Mon Sep 17 00:00:00 2001 From: Otto Winter Date: Thu, 26 Apr 2018 17:52:10 +0200 Subject: [PATCH 2/3] Fix test --- tests/components/mqtt/test_init.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/components/mqtt/test_init.py b/tests/components/mqtt/test_init.py index 5fe633e1964222..05c5de71b8cb07 100644 --- a/tests/components/mqtt/test_init.py +++ b/tests/components/mqtt/test_init.py @@ -155,7 +155,7 @@ def test_validate_subscribe_topic(self): """Test invalid subscribe topics.""" mqtt.valid_subscribe_topic('#') mqtt.valid_subscribe_topic('sport/#') - mqtt.valid_subscribe_topic('sport/#/') + self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'sport/#/') self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'foo/bar#') self.assertRaises(vol.Invalid, mqtt.valid_subscribe_topic, 'foo/#/bar') From 5d62419716fa5a5b269df567cb1e4f98fa68f456 Mon Sep 17 00:00:00 2001 From: Otto Winter Date: Fri, 27 Apr 2018 08:32:33 +0200 Subject: [PATCH 3/3] Improve length check --- homeassistant/components/mqtt/__init__.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/homeassistant/components/mqtt/__init__.py b/homeassistant/components/mqtt/__init__.py index 1968865eb0ab99..55d99a0817e1fe 100644 --- a/homeassistant/components/mqtt/__init__.py +++ b/homeassistant/components/mqtt/__init__.py @@ -97,7 +97,11 @@ def valid_topic(value: Any) -> str: raw_value = value.encode('utf-8') except UnicodeError: raise vol.Invalid("MQTT topic name/filter must be valid UTF-8 string.") - vol.Length(min=1, max=65535)(raw_value) + if not raw_value: + raise vol.Invalid("MQTT topic name/filter must not be empty.") + if len(raw_value) > 65535: + raise vol.Invalid("MQTT topic name/filter must not be longer than " + "65535 encoded bytes.") if '\0' in value: raise vol.Invalid("MQTT topic name/filter must not contain null " "character.")