Fix swallowed exceptions in action handlers for Pushover (#177416)

homeassistant/components/pushover/notify.py

@@ -13,7 +13,7 @@ from homeassistant.components.notify import (
     BaseNotificationService,
 )
 from homeassistant.core import HomeAssistant
-from homeassistant.exceptions import HomeAssistantError
+from homeassistant.exceptions import HomeAssistantError, ServiceValidationError
 from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType
 
 from .const import (
@@ -30,6 +30,7 @@ from .const import (
     ATTR_URL,
     ATTR_URL_TITLE,
     CONF_USER_KEY,
+    DOMAIN,
 )
 
 _LOGGER = logging.getLogger(__name__)
@@ -97,22 +98,23 @@ class PushoverNotificationService(BaseNotificationService):
         # Check for attachment
         if (image := data.get(ATTR_ATTACHMENT)) is not None:
             # Only allow attachments from whitelisted paths, check valid path
-            if self._hass.config.is_allowed_path(data[ATTR_ATTACHMENT]):
-                # try to open it as a normal file.
-                try:
-                    # pylint: disable-next=consider-using-with
-                    file_handle = open(data[ATTR_ATTACHMENT], "rb")
-                    # Replace the attachment identifier with file object.
-                    image = file_handle
-                # pylint: disable-next=home-assistant-action-swallowed-exception
-                except OSError as ex_val:
-                    _LOGGER.error(ex_val)
-                    # Remove attachment key to send without attachment.
-                    image = None
-            else:
-                _LOGGER.error("Path is not whitelisted")
-                # Remove attachment key to send without attachment.
-                image = None
+            if not self._hass.config.is_allowed_path(data[ATTR_ATTACHMENT]):
+                raise ServiceValidationError(
+                    translation_domain=DOMAIN,
+                    translation_key="attachment_not_allowed",
+                    translation_placeholders={"attachment": data[ATTR_ATTACHMENT]},
+                )
+            try:
+                # pylint: disable-next=consider-using-with
+                file_handle = open(data[ATTR_ATTACHMENT], "rb")
+            except OSError as err:
+                raise ServiceValidationError(
+                    translation_domain=DOMAIN,
+                    translation_key="attachment_open_failed",
+                    translation_placeholders={"attachment": data[ATTR_ATTACHMENT]},
+                ) from err
+            # Replace the attachment identifier with file object.
+            image = file_handle
 
         try:
             result = self.pushover.send_message(

homeassistant/components/pushover/strings.json

@@ -25,6 +25,14 @@
       }
     }
   },
+  "exceptions": {
+    "attachment_not_allowed": {
+      "message": "Attachment path {attachment} is not allowed."
+    },
+    "attachment_open_failed": {
+      "message": "Failed to open attachment {attachment}."
+    }
+  },
   "services": {
     "cancel": {
       "description": "Cancels one or more Pushover emergency notifications (priority 2) that were previously sent through the targeted Pushover account.",

tests/components/pushover/test_notify.py

@@ -1,5 +1,6 @@
 """Test the pushover notify platform."""
 
+from pathlib import Path
 from unittest.mock import MagicMock, patch
 
 from pushover_complete import BadAPIRequestError
@@ -94,6 +95,78 @@ async def test_send_message(
     )
 
 
+@pytest.mark.usefixtures("mock_pushover")
+@pytest.mark.parametrize(
+    ("is_allowed", "translation_key"),
+    [
+        pytest.param(False, "attachment_not_allowed", id="not_allowed"),
+        pytest.param(True, "attachment_open_failed", id="open_failed"),
+    ],
+)
+async def test_send_message_attachment_error(
+    hass: HomeAssistant,
+    mock_send_message: MagicMock,
+    is_allowed: bool,
+    translation_key: str,
+) -> None:
+    """Test that an unusable attachment raises and sends nothing."""
+    entry = MockConfigEntry(domain=DOMAIN, data=MOCK_CONFIG)
+    entry.add_to_hass(hass)
+
+    await hass.config_entries.async_setup(entry.entry_id)
+    await hass.async_block_till_done()
+
+    with (
+        patch.object(hass.config, "is_allowed_path", return_value=is_allowed),
+        pytest.raises(ServiceValidationError) as exc_info,
+    ):
+        await hass.services.async_call(
+            "notify",
+            "pushover",
+            {
+                "message": "Hello",
+                "data": {"attachment": "/nonexistent/attachment.jpg"},
+            },
+            blocking=True,
+        )
+
+    assert exc_info.value.translation_domain == DOMAIN
+    assert exc_info.value.translation_key == translation_key
+    mock_send_message.assert_not_called()
+
+
+@pytest.mark.usefixtures("mock_pushover")
+async def test_send_message_with_attachment(
+    hass: HomeAssistant,
+    mock_send_message: MagicMock,
+    tmp_path: Path,
+) -> None:
+    """Test that a readable attachment is sent as an open file."""
+    attachment = tmp_path / "attachment.jpg"
+    attachment.write_bytes(b"image data")
+
+    entry = MockConfigEntry(domain=DOMAIN, data=MOCK_CONFIG)
+    entry.add_to_hass(hass)
+
+    await hass.config_entries.async_setup(entry.entry_id)
+    await hass.async_block_till_done()
+
+    with patch.object(hass.config, "is_allowed_path", return_value=True):
+        await hass.services.async_call(
+            "notify",
+            "pushover",
+            {
+                "message": "Hello",
+                "data": {"attachment": str(attachment)},
+            },
+            blocking=True,
+        )
+
+    image = mock_send_message.call_args.kwargs["image"]
+    assert image.name == str(attachment)
+    image.close()
+
+
 async def test_cancel_by_tag(
     hass: HomeAssistant,
     mock_pushover: MagicMock,