Add missing timeout to command_line platforms: cover, notify, switch (#38497)

* Add missing timeout to command_line platforms: cover, notify, switch

* add timeout test for notify

homeassistant/components/command_line/__init__.py

@@ -1 +1,42 @@
 """The command_line component."""
+
+import logging
+import subprocess
+
+_LOGGER = logging.getLogger(__name__)
+
+
+def call_shell_with_timeout(command, timeout):
+    """Run a shell command with a timeout."""
+    try:
+        _LOGGER.debug("Running command: %s", command)
+        subprocess.check_output(
+            command, shell=True, timeout=timeout  # nosec # shell by design
+        )
+        return 0
+    except subprocess.CalledProcessError as proc_exception:
+        _LOGGER.error("Command failed: %s", command)
+        return proc_exception.returncode
+    except subprocess.TimeoutExpired:
+        _LOGGER.error("Timeout for command: %s", command)
+        return -1
+    except subprocess.SubprocessError:
+        _LOGGER.error("Error trying to exec command: %s", command)
+        return -1
+
+
+def check_output_or_log(command, timeout):
+    """Run a shell command with a timeout and return the output."""
+    try:
+        return_value = subprocess.check_output(
+            command, shell=True, timeout=timeout  # nosec # shell by design
+        )
+        return return_value.strip().decode("utf-8")
+    except subprocess.CalledProcessError:
+        _LOGGER.error("Command failed: %s", command)
+    except subprocess.TimeoutExpired:
+        _LOGGER.error("Timeout for command: %s", command)
+    except subprocess.SubprocessError:
+        _LOGGER.error("Error trying to exec command: %s", command)
+
+    return None

homeassistant/components/command_line/binary_sensor.py

@@ -19,6 +19,7 @@ from homeassistant.const import (
 )
 import homeassistant.helpers.config_validation as cv
 
+from .const import CONF_COMMAND_TIMEOUT, DEFAULT_TIMEOUT
 from .sensor import CommandSensorData
 
 _LOGGER = logging.getLogger(__name__)
@@ -29,8 +30,6 @@ DEFAULT_PAYLOAD_OFF = "OFF"
 
 SCAN_INTERVAL = timedelta(seconds=60)
 
-CONF_COMMAND_TIMEOUT = "command_timeout"
-DEFAULT_TIMEOUT = 15
 
 PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend(
     {

homeassistant/components/command_line/const.py

@@ -0,0 +1,4 @@
+"""Allows to configure custom shell commands to turn a value for a sensor."""
+
+CONF_COMMAND_TIMEOUT = "command_timeout"
+DEFAULT_TIMEOUT = 15

homeassistant/components/command_line/cover.py

@@ -1,6 +1,5 @@
 """Support for command line covers."""
 import logging
-import subprocess
 
 import voluptuous as vol
 
@@ -16,6 +15,9 @@ from homeassistant.const import (
 )
 import homeassistant.helpers.config_validation as cv
 
+from . import call_shell_with_timeout, check_output_or_log
+from .const import CONF_COMMAND_TIMEOUT, DEFAULT_TIMEOUT
+
 _LOGGER = logging.getLogger(__name__)
 
 COVER_SCHEMA = vol.Schema(
@@ -26,6 +28,7 @@ COVER_SCHEMA = vol.Schema(
         vol.Optional(CONF_COMMAND_STOP, default="true"): cv.string,
         vol.Optional(CONF_FRIENDLY_NAME): cv.string,
         vol.Optional(CONF_VALUE_TEMPLATE): cv.template,
+        vol.Optional(CONF_COMMAND_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,
     }
 )
 
@@ -48,11 +51,12 @@ def setup_platform(hass, config, add_entities, discovery_info=None):
             CommandCover(
                 hass,
                 device_config.get(CONF_FRIENDLY_NAME, device_name),
-                device_config.get(CONF_COMMAND_OPEN),
-                device_config.get(CONF_COMMAND_CLOSE),
-                device_config.get(CONF_COMMAND_STOP),
+                device_config[CONF_COMMAND_OPEN],
+                device_config[CONF_COMMAND_CLOSE],
+                device_config[CONF_COMMAND_STOP],
                 device_config.get(CONF_COMMAND_STATE),
                 value_template,
+                device_config[CONF_COMMAND_TIMEOUT],
             )
         )
 
@@ -75,6 +79,7 @@ class CommandCover(CoverEntity):
         command_stop,
         command_state,
         value_template,
+        timeout,
     ):
         """Initialize the cover."""
         self._hass = hass
@@ -85,31 +90,23 @@ class CommandCover(CoverEntity):
         self._command_stop = command_stop
         self._command_state = command_state
         self._value_template = value_template
+        self._timeout = timeout
 
-    @staticmethod
-    def _move_cover(command):
+    def _move_cover(self, command):
         """Execute the actual commands."""
         _LOGGER.info("Running command: %s", command)
 
-        success = subprocess.call(command, shell=True) == 0  # nosec # shell by design
+        success = call_shell_with_timeout(command, self._timeout) == 0
 
         if not success:
             _LOGGER.error("Command failed: %s", command)
 
         return success
 
-    @staticmethod
-    def _query_state_value(command):
+    def _query_state_value(self, command):
         """Execute state command for return value."""
-        _LOGGER.info("Running state command: %s", command)
-
-        try:
-            return_value = subprocess.check_output(
-                command, shell=True  # nosec # shell by design
-            )
-            return return_value.strip().decode("utf-8")
-        except subprocess.CalledProcessError:
-            _LOGGER.error("Command failed: %s", command)
+        _LOGGER.info("Running state value command: %s", command)
+        return check_output_or_log(command, self._timeout)
 
     @property
     def should_poll(self):

homeassistant/components/command_line/notify.py

@@ -8,26 +8,34 @@ from homeassistant.components.notify import PLATFORM_SCHEMA, BaseNotificationSer
 from homeassistant.const import CONF_COMMAND, CONF_NAME
 import homeassistant.helpers.config_validation as cv
 
+from .const import CONF_COMMAND_TIMEOUT, DEFAULT_TIMEOUT
+
 _LOGGER = logging.getLogger(__name__)
 
 PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend(
-    {vol.Required(CONF_COMMAND): cv.string, vol.Optional(CONF_NAME): cv.string}
+    {
+        vol.Required(CONF_COMMAND): cv.string,
+        vol.Optional(CONF_NAME): cv.string,
+        vol.Optional(CONF_COMMAND_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,
+    }
 )
 
 
 def get_service(hass, config, discovery_info=None):
     """Get the Command Line notification service."""
     command = config[CONF_COMMAND]
+    timeout = config[CONF_COMMAND_TIMEOUT]
 
-    return CommandLineNotificationService(command)
+    return CommandLineNotificationService(command, timeout)
 
 
 class CommandLineNotificationService(BaseNotificationService):
     """Implement the notification service for the Command Line service."""
 
-    def __init__(self, command):
+    def __init__(self, command, timeout):
         """Initialize the service."""
         self.command = command
+        self._timeout = timeout
 
     def send_message(self, message="", **kwargs):
         """Send a message to a command line."""
@@ -38,8 +46,10 @@ class CommandLineNotificationService(BaseNotificationService):
                 stdin=subprocess.PIPE,
                 shell=True,  # nosec # shell by design
             )
-            proc.communicate(input=message)
+            proc.communicate(input=message, timeout=self._timeout)
             if proc.returncode != 0:
                 _LOGGER.error("Command failed: %s", self.command)
+        except subprocess.TimeoutExpired:
+            _LOGGER.error("Timeout for command: %s", self.command)
         except subprocess.SubprocessError:
-            _LOGGER.error("Error trying to exec Command: %s", self.command)
+            _LOGGER.error("Error trying to exec command: %s", self.command)

homeassistant/components/command_line/sensor.py

@@ -3,7 +3,6 @@ from collections.abc import Mapping
 from datetime import timedelta
 import json
 import logging
-import subprocess
 
 import voluptuous as vol
 
@@ -20,13 +19,14 @@ from homeassistant.helpers import template
 import homeassistant.helpers.config_validation as cv
 from homeassistant.helpers.entity import Entity
 
+from . import check_output_or_log
+from .const import CONF_COMMAND_TIMEOUT, DEFAULT_TIMEOUT
+
 _LOGGER = logging.getLogger(__name__)
 
-CONF_COMMAND_TIMEOUT = "command_timeout"
 CONF_JSON_ATTRIBUTES = "json_attributes"
 
 DEFAULT_NAME = "Command Sensor"
-DEFAULT_TIMEOUT = 15
 
 SCAN_INTERVAL = timedelta(seconds=60)
 
@@ -171,13 +171,6 @@ class CommandSensorData:
         else:
             # Template used. Construct the string used in the shell
             command = f"{prog} {rendered_args}"
-        try:
+
         _LOGGER.debug("Running command: %s", command)
-            return_value = subprocess.check_output(
-                command, shell=True, timeout=self.timeout  # nosec # shell by design
-            )
-            self.value = return_value.strip().decode("utf-8")
-        except subprocess.CalledProcessError:
-            _LOGGER.error("Command failed: %s", command)
-        except subprocess.TimeoutExpired:
-            _LOGGER.error("Timeout for command: %s", command)
+        self.value = check_output_or_log(command, self.timeout)

homeassistant/components/command_line/switch.py

@@ -1,6 +1,5 @@
 """Support for custom shell commands to turn a switch on/off."""
 import logging
-import subprocess
 
 import voluptuous as vol
 
@@ -19,6 +18,9 @@ from homeassistant.const import (
 )
 import homeassistant.helpers.config_validation as cv
 
+from . import call_shell_with_timeout, check_output_or_log
+from .const import CONF_COMMAND_TIMEOUT, DEFAULT_TIMEOUT
+
 _LOGGER = logging.getLogger(__name__)
 
 SWITCH_SCHEMA = vol.Schema(
@@ -28,6 +30,7 @@ SWITCH_SCHEMA = vol.Schema(
         vol.Optional(CONF_COMMAND_STATE): cv.string,
         vol.Optional(CONF_FRIENDLY_NAME): cv.string,
         vol.Optional(CONF_VALUE_TEMPLATE): cv.template,
+        vol.Optional(CONF_COMMAND_TIMEOUT, default=DEFAULT_TIMEOUT): cv.positive_int,
     }
 )
 
@@ -52,10 +55,11 @@ def setup_platform(hass, config, add_entities, discovery_info=None):
                 hass,
                 object_id,
                 device_config.get(CONF_FRIENDLY_NAME, object_id),
-                device_config.get(CONF_COMMAND_ON),
-                device_config.get(CONF_COMMAND_OFF),
+                device_config[CONF_COMMAND_ON],
+                device_config[CONF_COMMAND_OFF],
                 device_config.get(CONF_COMMAND_STATE),
                 value_template,
+                device_config[CONF_COMMAND_TIMEOUT],
             )
         )
 
@@ -78,6 +82,7 @@ class CommandSwitch(SwitchEntity):
         command_off,
         command_state,
         value_template,
+        timeout,
     ):
         """Initialize the switch."""
         self._hass = hass
@@ -88,37 +93,28 @@ class CommandSwitch(SwitchEntity):
         self._command_off = command_off
         self._command_state = command_state
         self._value_template = value_template
+        self._timeout = timeout
 
-    @staticmethod
-    def _switch(command):
+    def _switch(self, command):
         """Execute the actual commands."""
         _LOGGER.info("Running command: %s", command)
 
-        success = subprocess.call(command, shell=True) == 0  # nosec # shell by design
+        success = call_shell_with_timeout(command, self._timeout) == 0
 
         if not success:
             _LOGGER.error("Command failed: %s", command)
 
         return success
 
-    @staticmethod
-    def _query_state_value(command):
+    def _query_state_value(self, command):
         """Execute state command for return value."""
-        _LOGGER.info("Running state command: %s", command)
+        _LOGGER.info("Running state value command: %s", command)
+        return check_output_or_log(command, self._timeout)
 
-        try:
-            return_value = subprocess.check_output(
-                command, shell=True  # nosec # shell by design
-            )
-            return return_value.strip().decode("utf-8")
-        except subprocess.CalledProcessError:
-            _LOGGER.error("Command failed: %s", command)
-
-    @staticmethod
-    def _query_state_code(command):
+    def _query_state_code(self, command):
         """Execute state command for return code."""
-        _LOGGER.info("Running state command: %s", command)
-        return subprocess.call(command, shell=True) == 0  # nosec # shell by design
+        _LOGGER.info("Running state code command: %s", command)
+        return call_shell_with_timeout(command, self._timeout) == 0
 
     @property
     def should_poll(self):
@@ -146,8 +142,8 @@ class CommandSwitch(SwitchEntity):
             _LOGGER.error("No state command specified")
             return
         if self._value_template:
-            return CommandSwitch._query_state_value(self._command_state)
-        return CommandSwitch._query_state_code(self._command_state)
+            return self._query_state_value(self._command_state)
+        return self._query_state_code(self._command_state)
 
     def update(self):
         """Update device state."""
@@ -159,12 +155,12 @@ class CommandSwitch(SwitchEntity):
 
     def turn_on(self, **kwargs):
         """Turn the device on."""
-        if CommandSwitch._switch(self._command_on) and not self._command_state:
+        if self._switch(self._command_on) and not self._command_state:
             self._state = True
             self.schedule_update_ha_state()
 
     def turn_off(self, **kwargs):
         """Turn the device off."""
-        if CommandSwitch._switch(self._command_off) and not self._command_state:
+        if self._switch(self._command_off) and not self._command_state:
             self._state = False
             self.schedule_update_ha_state()

tests/components/command_line/test_cover.py

@@ -27,6 +27,7 @@ def rs(hass):
         "command_stop",
         "command_state",
         None,
+        15,
     )
 
 
@@ -45,7 +46,7 @@ def test_query_state_value(rs):
         assert "foo bar" == result
         assert mock_run.call_count == 1
         assert mock_run.call_args == mock.call(
-            "runme", shell=True,  # nosec # shell by design
+            "runme", shell=True, timeout=15  # nosec # shell by design
         )
 
 

tests/components/command_line/test_notify.py

@@ -4,7 +4,7 @@ import tempfile
 import unittest
 
 import homeassistant.components.notify as notify
-from homeassistant.setup import setup_component
+from homeassistant.setup import async_setup_component, setup_component
 
 from tests.async_mock import patch
 from tests.common import assert_setup_component, get_test_home_assistant
@@ -93,3 +93,25 @@ class TestCommandLine(unittest.TestCase):
             "notify", "test", {"message": "error"}, blocking=True
         )
         assert mock_error.call_count == 1
+
+
+async def test_timeout(hass, caplog):
+    """Test we do not block forever."""
+    assert await async_setup_component(
+        hass,
+        notify.DOMAIN,
+        {
+            "notify": {
+                "name": "test",
+                "platform": "command_line",
+                "command": "sleep 10000",
+                "command_timeout": 0.0000001,
+            }
+        },
+    )
+    await hass.async_block_till_done()
+    assert await hass.services.async_call(
+        "notify", "test", {"message": "error"}, blocking=True
+    )
+    await hass.async_block_till_done()
+    assert "Timeout" in caplog.text

tests/components/command_line/test_sensor.py

@@ -74,7 +74,7 @@ class TestCommandSensorSensor(unittest.TestCase):
         """Ensure command with templates and quotes get rendered properly."""
         self.hass.states.set("sensor.test_state", "Works 2")
         with patch(
-            "homeassistant.components.command_line.sensor.subprocess.check_output",
+            "homeassistant.components.command_line.subprocess.check_output",
             return_value=b"Works\n",
         ) as check_output:
             data = command_line.CommandSensorData(

tests/components/command_line/test_switch.py

@@ -180,13 +180,14 @@ class TestCommandSwitch(unittest.TestCase):
             "echo 'off command'",
             None,
             None,
+            15,
         ]
 
         no_state_device = command_line.CommandSwitch(*init_args)
         assert no_state_device.assumed_state
 
         # Set state command
-        init_args[-2] = "cat {}"
+        init_args[-3] = "cat {}"
 
         state_device = command_line.CommandSwitch(*init_args)
         assert not state_device.assumed_state
@@ -201,6 +202,7 @@ class TestCommandSwitch(unittest.TestCase):
             "echo 'off command'",
             False,
             None,
+            15,
         ]
 
         test_switch = command_line.CommandSwitch(*init_args)