From 6ee185c58aa047dde7e933c4d5a1b47c42f6572a Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 9 Feb 2026 03:25:23 -0600 Subject: [PATCH] [dashboard] Use resolve/relative_to for download path validation (#13867) --- esphome/dashboard/web_server.py | 17 ++++-- tests/dashboard/test_web_server.py | 93 +++++++++++++++++++++++++----- 2 files changed, 92 insertions(+), 18 deletions(-) diff --git a/esphome/dashboard/web_server.py b/esphome/dashboard/web_server.py index da50279864..00974bf460 100644 --- a/esphome/dashboard/web_server.py +++ b/esphome/dashboard/web_server.py @@ -1054,17 +1054,26 @@ class DownloadBinaryRequestHandler(BaseHandler): # fallback to type=, but prioritize file= file_name = self.get_argument("type", None) file_name = self.get_argument("file", file_name) - if file_name is None: + if file_name is None or not file_name.strip(): self.send_error(400) return - file_name = file_name.replace("..", "").lstrip("/") # get requested download name, or build it based on filename download_name = self.get_argument( "download", f"{storage_json.name}-{file_name}", ) - path = storage_json.firmware_bin_path.parent.joinpath(file_name) + if storage_json.firmware_bin_path is None: + self.send_error(404) + return + + base_dir = storage_json.firmware_bin_path.parent.resolve() + path = base_dir.joinpath(file_name).resolve() + try: + path.relative_to(base_dir) + except ValueError: + self.send_error(403) + return if not path.is_file(): args = ["esphome", "idedata", settings.rel_path(configuration)] @@ -1078,7 +1087,7 @@ class DownloadBinaryRequestHandler(BaseHandler): found = False for image in idedata.extra_flash_images: - if image.path.endswith(file_name): + if image.path.as_posix().endswith(file_name): path = image.path download_name = file_name found = True diff --git a/tests/dashboard/test_web_server.py b/tests/dashboard/test_web_server.py index 7642876ee5..9ea7a5164b 100644 --- a/tests/dashboard/test_web_server.py +++ b/tests/dashboard/test_web_server.py @@ -8,6 +8,7 @@ import gzip import json import os from pathlib import Path +import sys from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest @@ -421,7 +422,7 @@ async def test_download_binary_handler_idedata_fallback( # Mock idedata response mock_image = Mock() - mock_image.path = str(bootloader_file) + mock_image.path = bootloader_file mock_idedata_instance = Mock() mock_idedata_instance.extra_flash_images = [mock_image] mock_idedata.return_value = mock_idedata_instance @@ -528,14 +529,22 @@ async def test_download_binary_handler_subdirectory_file_url_encoded( @pytest.mark.asyncio @pytest.mark.usefixtures("mock_ext_storage_path") @pytest.mark.parametrize( - "attack_path", + ("attack_path", "expected_code"), [ - pytest.param("../../../secrets.yaml", id="basic_traversal"), - pytest.param("..%2F..%2F..%2Fsecrets.yaml", id="url_encoded"), - pytest.param("zephyr/../../../secrets.yaml", id="traversal_with_prefix"), - pytest.param("/etc/passwd", id="absolute_path"), - pytest.param("//etc/passwd", id="double_slash_absolute"), - pytest.param("....//secrets.yaml", id="multiple_dots"), + pytest.param("../../../secrets.yaml", 403, id="basic_traversal"), + pytest.param("..%2F..%2F..%2Fsecrets.yaml", 403, id="url_encoded"), + pytest.param("zephyr/../../../secrets.yaml", 403, id="traversal_with_prefix"), + pytest.param("/etc/passwd", 403, id="absolute_path"), + pytest.param("//etc/passwd", 403, id="double_slash_absolute"), + pytest.param( + "....//secrets.yaml", + # On Windows, Path.resolve() treats "..." and "...." as parent + # traversal (like ".."), so the path escapes base_dir -> 403. + # On Unix, "...." is a literal directory name that stays inside + # base_dir but doesn't exist -> 404. + 403 if sys.platform == "win32" else 404, + id="multiple_dots", + ), ], ) async def test_download_binary_handler_path_traversal_protection( @@ -543,11 +552,14 @@ async def test_download_binary_handler_path_traversal_protection( tmp_path: Path, mock_storage_json: MagicMock, attack_path: str, + expected_code: int, ) -> None: """Test that DownloadBinaryRequestHandler prevents path traversal attacks. - Verifies that attempts to use '..' in file paths are sanitized to prevent - accessing files outside the build directory. Tests multiple attack vectors. + Verifies that attempts to escape the build directory via '..' are rejected + using resolve()/relative_to() validation. Tests multiple attack vectors. + Real traversals that escape the base directory get 403. Paths like '....' + that resolve inside the base directory but don't exist get 404. """ # Create build structure build_dir = get_build_path(tmp_path, "test") @@ -565,14 +577,67 @@ async def test_download_binary_handler_path_traversal_protection( mock_storage.firmware_bin_path = firmware_file mock_storage_json.load.return_value = mock_storage - # Attempt path traversal attack - should be blocked - with pytest.raises(HTTPClientError) as exc_info: + # Mock async_run_system_command so paths that pass validation but don't exist + # return 404 deterministically without spawning a real subprocess. + with ( + patch( + "esphome.dashboard.web_server.async_run_system_command", + new_callable=AsyncMock, + return_value=(2, "", ""), + ), + pytest.raises(HTTPClientError) as exc_info, + ): await dashboard.fetch( f"/download.bin?configuration=test.yaml&file={attack_path}", method="GET", ) - # Should get 404 (file not found after sanitization) or 500 (idedata fails) - assert exc_info.value.code in (404, 500) + assert exc_info.value.code == expected_code + + +@pytest.mark.asyncio +@pytest.mark.usefixtures("mock_ext_storage_path") +async def test_download_binary_handler_no_firmware_bin_path( + dashboard: DashboardTestHelper, + mock_storage_json: MagicMock, +) -> None: + """Test that download returns 404 when firmware_bin_path is None. + + This covers configs created by StorageJSON.from_wizard() where no + firmware has been compiled yet. + """ + mock_storage = Mock() + mock_storage.name = "test_device" + mock_storage.firmware_bin_path = None + mock_storage_json.load.return_value = mock_storage + + with pytest.raises(HTTPClientError) as exc_info: + await dashboard.fetch( + "/download.bin?configuration=test.yaml&file=firmware.bin", + method="GET", + ) + assert exc_info.value.code == 404 + + +@pytest.mark.asyncio +@pytest.mark.usefixtures("mock_ext_storage_path") +@pytest.mark.parametrize("file_value", ["", "%20%20", "%20"]) +async def test_download_binary_handler_empty_file_name( + dashboard: DashboardTestHelper, + mock_storage_json: MagicMock, + file_value: str, +) -> None: + """Test that download returns 400 for empty or whitespace-only file names.""" + mock_storage = Mock() + mock_storage.name = "test_device" + mock_storage.firmware_bin_path = Path("/fake/firmware.bin") + mock_storage_json.load.return_value = mock_storage + + with pytest.raises(HTTPClientError) as exc_info: + await dashboard.fetch( + f"/download.bin?configuration=test.yaml&file={file_value}", + method="GET", + ) + assert exc_info.value.code == 400 @pytest.mark.asyncio