From 9b2d17cd0082caa58f0153262b9b51aa10e32d72 Mon Sep 17 00:00:00 2001 From: uvjustin <46082645+uvjustin@users.noreply.github.com> Date: Fri, 9 Sep 2022 19:18:24 +0800 Subject: [PATCH] Escape media_content_id in media player proxy (#77811) * Escape media_content_id in media player proxy * Change usage in kodi * Change usage in roku * Change usage in sonos * Add test * Add comment * Change path regex instead of double quoting * Use .+ instead of .* --- homeassistant/components/kodi/media_player.py | 3 +- .../components/media_player/__init__.py | 11 +++++-- homeassistant/components/roku/browse_media.py | 3 +- .../components/sonos/media_browser.py | 5 ++- tests/components/media_player/test_init.py | 31 +++++++++++++++++++ 5 files changed, 43 insertions(+), 10 deletions(-) diff --git a/homeassistant/components/kodi/media_player.py b/homeassistant/components/kodi/media_player.py index 80331794114..a41738eaa3e 100644 --- a/homeassistant/components/kodi/media_player.py +++ b/homeassistant/components/kodi/media_player.py @@ -7,7 +7,6 @@ from functools import wraps import logging import re from typing import Any, TypeVar -import urllib.parse from jsonrpc_base.jsonrpc import ProtocolError, TransportError from pykodi import CannotConnectError @@ -920,7 +919,7 @@ class KodiEntity(MediaPlayerEntity): return self.get_browse_image_url( media_content_type, - urllib.parse.quote_plus(media_content_id), + media_content_id, media_image_id, ) diff --git a/homeassistant/components/media_player/__init__.py b/homeassistant/components/media_player/__init__.py index 2c24a43ffc3..177d7f5ad74 100644 --- a/homeassistant/components/media_player/__init__.py +++ b/homeassistant/components/media_player/__init__.py @@ -13,7 +13,7 @@ from http import HTTPStatus import logging import secrets from typing import Any, cast, final -from urllib.parse import urlparse +from urllib.parse import quote, urlparse from aiohttp import web from aiohttp.hdrs import CACHE_CONTROL, CONTENT_TYPE @@ -1100,7 +1100,9 @@ class MediaPlayerEntity(Entity): """Generate an url for a media browser image.""" url_path = ( f"/api/media_player_proxy/{self.entity_id}/browse_media" - f"/{media_content_type}/{media_content_id}" + # quote the media_content_id as it may contain url unsafe characters + # aiohttp will unquote the path automatically + f"/{media_content_type}/{quote(media_content_id)}" ) url_query = {"token": self.access_token} @@ -1117,7 +1119,10 @@ class MediaPlayerImageView(HomeAssistantView): url = "/api/media_player_proxy/{entity_id}" name = "api:media_player:image" extra_urls = [ - url + "/browse_media/{media_content_type}/{media_content_id}", + # Need to modify the default regex for media_content_id as it may + # include arbitrary characters including '/','{', or '}' + url + + "/browse_media/{media_content_type}/{media_content_id:.+}", ] def __init__(self, component: EntityComponent) -> None: diff --git a/homeassistant/components/roku/browse_media.py b/homeassistant/components/roku/browse_media.py index 72b572e8d3e..495ff910fd9 100644 --- a/homeassistant/components/roku/browse_media.py +++ b/homeassistant/components/roku/browse_media.py @@ -3,7 +3,6 @@ from __future__ import annotations from collections.abc import Callable from functools import partial -from urllib.parse import quote_plus from homeassistant.components import media_source from homeassistant.components.media_player import BrowseMedia @@ -64,7 +63,7 @@ def get_thumbnail_url_full( return get_browse_image_url( media_content_type, - quote_plus(media_content_id), + media_content_id, media_image_id, ) diff --git a/homeassistant/components/sonos/media_browser.py b/homeassistant/components/sonos/media_browser.py index 95ff08cb87b..3859f691179 100644 --- a/homeassistant/components/sonos/media_browser.py +++ b/homeassistant/components/sonos/media_browser.py @@ -6,7 +6,6 @@ from contextlib import suppress from functools import partial import logging from typing import cast -from urllib.parse import quote_plus, unquote from soco.data_structures import DidlFavorite, DidlObject from soco.ms_data_structures import MusicServiceItem @@ -64,7 +63,7 @@ def get_thumbnail_url_full( return get_browse_image_url( media_content_type, - quote_plus(media_content_id), + media_content_id, media_image_id, ) @@ -201,7 +200,7 @@ def build_item_response( if not title: try: - title = unquote(payload["idstring"].split("/")[1]) + title = payload["idstring"].split("/")[1] except IndexError: title = LIBRARY_TITLES_MAPPING[payload["idstring"]] diff --git a/tests/components/media_player/test_init.py b/tests/components/media_player/test_init.py index f0666a11545..e7946d447e9 100644 --- a/tests/components/media_player/test_init.py +++ b/tests/components/media_player/test_init.py @@ -280,3 +280,34 @@ async def test_enqueue_alert_exclusive(hass): }, blocking=True, ) + + +async def test_get_async_get_browse_image_quoting( + hass, hass_client_no_auth, hass_ws_client +): + """Test get browse image using media_content_id with special characters. + + async_get_browse_image() should get called with the same string that is + passed into get_browse_image_url(). + """ + await async_setup_component( + hass, "media_player", {"media_player": {"platform": "demo"}} + ) + await hass.async_block_till_done() + + entity_comp = hass.data.get("entity_components", {}).get("media_player") + assert entity_comp + + player = entity_comp.get_entity("media_player.bedroom") + assert player + + client = await hass_client_no_auth() + + with patch( + "homeassistant.components.media_player.MediaPlayerEntity." + "async_get_browse_image", + ) as mock_browse_image: + media_content_id = "a/b c/d+e%2Fg{}" + url = player.get_browse_image_url("album", media_content_id) + await client.get(url) + mock_browse_image.assert_called_with("album", media_content_id, None)