From cc126abfb2c1ac27b7aeb7f9d66aac523d5ec61a Mon Sep 17 00:00:00 2001 From: Martin <32802427+mstu01@users.noreply.github.com> Date: Sun, 30 Aug 2026 21:14:45 +0200 Subject: [PATCH] Fix swallowed exceptions in action handlers for Pushover (#177416) --- homeassistant/components/pushover/notify.py | 36 ++++----- .../components/pushover/strings.json | 8 ++ tests/components/pushover/test_notify.py | 73 +++++++++++++++++++ 3 files changed, 100 insertions(+), 17 deletions(-) diff --git a/homeassistant/components/pushover/notify.py b/homeassistant/components/pushover/notify.py index ee4a95837898..868083a3879e 100644 --- a/homeassistant/components/pushover/notify.py +++ b/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( diff --git a/homeassistant/components/pushover/strings.json b/homeassistant/components/pushover/strings.json index 24c9d58440b8..ad7adaaf527b 100644 --- a/homeassistant/components/pushover/strings.json +++ b/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.", diff --git a/tests/components/pushover/test_notify.py b/tests/components/pushover/test_notify.py index 49ee1a93d803..39a2ed482c59 100644 --- a/tests/components/pushover/test_notify.py +++ b/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,