From f060c4020c9a2d6a1ebeeade0b5dc15f71e565b4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 18 Nov 2023 09:02:21 -0600 Subject: [PATCH 1/4] safe file writer --- esphome/dashboard/util/file.py | 43 +++++++++++++++++++++++++++ tests/dashboard/__init__.py | 0 tests/dashboard/util/__init__.py | 0 tests/dashboard/util/test_file.py | 48 +++++++++++++++++++++++++++++++ 4 files changed, 91 insertions(+) create mode 100644 esphome/dashboard/util/file.py create mode 100644 tests/dashboard/__init__.py create mode 100644 tests/dashboard/util/__init__.py create mode 100644 tests/dashboard/util/test_file.py diff --git a/esphome/dashboard/util/file.py b/esphome/dashboard/util/file.py new file mode 100644 index 000000000..74deeacf8 --- /dev/null +++ b/esphome/dashboard/util/file.py @@ -0,0 +1,43 @@ +import logging +import os +import tempfile +from pathlib import Path + +_LOGGER = logging.getLogger(__name__) + + +# from https://github.com/home-assistant/core/blob/dev/homeassistant/util/file.py +def write_utf8_file( + filename: Path, + utf8_data: str, + private: bool = False, +) -> None: + """Write a file and rename it into place. + + Writes all or nothing. + """ + + tmp_filename = "" + try: + # Modern versions of Python tempfile create this file with mode 0o600 + with tempfile.NamedTemporaryFile( + mode="w", encoding="utf-8", dir=os.path.dirname(filename), delete=False + ) as fdesc: + fdesc.write(utf8_data) + tmp_filename = fdesc.name + if not private: + os.fchmod(fdesc.fileno(), 0o644) + os.replace(tmp_filename, filename) + finally: + if os.path.exists(tmp_filename): + try: + os.remove(tmp_filename) + except OSError as err: + # If we are cleaning up then something else went wrong, so + # we should suppress likely follow-on errors in the cleanup + _LOGGER.error( + "File replacement cleanup failed for %s while saving %s: %s", + tmp_filename, + filename, + err, + ) diff --git a/tests/dashboard/__init__.py b/tests/dashboard/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/dashboard/util/__init__.py b/tests/dashboard/util/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/dashboard/util/test_file.py b/tests/dashboard/util/test_file.py new file mode 100644 index 000000000..fd4860dbb --- /dev/null +++ b/tests/dashboard/util/test_file.py @@ -0,0 +1,48 @@ +import os +from pathlib import Path +from unittest.mock import patch + +import py +import pytest + +from esphome.dashboard.util.file import write_utf8_file + + +def test_write_utf8_file(tmp_path: Path) -> None: + write_utf8_file(tmp_path.joinpath("foo.txt"), "foo") + assert tmp_path.joinpath("foo.txt").read_text() == "foo" + + with pytest.raises(OSError): + write_utf8_file(Path("/not-writable"), "bar") + + +def test_write_utf8_file_fails_at_rename( + tmpdir: py.path.local, caplog: pytest.LogCaptureFixture +) -> None: + """Test that if rename fails not not remove, we do not log the failed cleanup.""" + test_dir = tmpdir.mkdir("files") + test_file = Path(test_dir / "test.json") + + with pytest.raises(OSError), patch( + "esphome.dashboard.util.file.os.replace", side_effect=OSError + ): + write_utf8_file(test_file, '{"some":"data"}', False) + + assert not os.path.exists(test_file) + + assert "File replacement cleanup failed" not in caplog.text + + +def test_write_utf8_file_fails_at_rename_and_remove( + tmpdir: py.path.local, caplog: pytest.LogCaptureFixture +) -> None: + """Test that if rename and remove both fail, we log the failed cleanup.""" + test_dir = tmpdir.mkdir("files") + test_file = Path(test_dir / "test.json") + + with pytest.raises(OSError), patch( + "esphome.dashboard.util.file.os.remove", side_effect=OSError + ), patch("esphome.dashboard.util.file.os.replace", side_effect=OSError): + write_utf8_file(test_file, '{"some":"data"}', False) + + assert "File replacement cleanup failed" in caplog.text From d46188ecdadb90bf41af12c8a50d84c8ea262429 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 18 Nov 2023 09:13:42 -0600 Subject: [PATCH 2/4] fixes --- esphome/dashboard/util/file.py | 18 ++++++++++++++--- esphome/dashboard/web_server.py | 33 +++++++++++++++++++++---------- tests/dashboard/util/test_file.py | 7 ++++++- 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/esphome/dashboard/util/file.py b/esphome/dashboard/util/file.py index 74deeacf8..5f3c5f5f1 100644 --- a/esphome/dashboard/util/file.py +++ b/esphome/dashboard/util/file.py @@ -6,10 +6,22 @@ from pathlib import Path _LOGGER = logging.getLogger(__name__) -# from https://github.com/home-assistant/core/blob/dev/homeassistant/util/file.py def write_utf8_file( filename: Path, - utf8_data: str, + utf8_str: str, + private: bool = False, +) -> None: + """Write a file and rename it into place. + + Writes all or nothing. + """ + write_file(filename, utf8_str.encode("utf-8"), private) + + +# from https://github.com/home-assistant/core/blob/dev/homeassistant/util/file.py +def write_file( + filename: Path, + utf8_data: bytes, private: bool = False, ) -> None: """Write a file and rename it into place. @@ -21,7 +33,7 @@ def write_utf8_file( try: # Modern versions of Python tempfile create this file with mode 0o600 with tempfile.NamedTemporaryFile( - mode="w", encoding="utf-8", dir=os.path.dirname(filename), delete=False + mode="wb", dir=os.path.dirname(filename), delete=False ) as fdesc: fdesc.write(utf8_data) tmp_filename = fdesc.name diff --git a/esphome/dashboard/web_server.py b/esphome/dashboard/web_server.py index aa8e44581..93d836d76 100644 --- a/esphome/dashboard/web_server.py +++ b/esphome/dashboard/web_server.py @@ -38,6 +38,7 @@ from esphome.yaml_util import FastestAvailableSafeLoader from .core import DASHBOARD from .entries import EntryState, entry_state_to_bool +from .util.file import write_file from .util.subprocess import async_run_system_command from .util.text import friendly_name_slugify @@ -746,22 +747,34 @@ class InfoRequestHandler(BaseHandler): class EditRequestHandler(BaseHandler): @authenticated @bind_config - def get(self, configuration=None): + async def get(self, configuration: str | None = None): + loop = asyncio.get_running_loop() filename = settings.rel_path(configuration) - content = "" - if os.path.isfile(filename): - with open(file=filename, encoding="utf-8") as f: - content = f.read() + try: + content = await loop.run_in_executor(None, self._read_file, filename) + except OSError: + self.send_error(404) + return self.write(content) + def _read_file(self, filename: str) -> bytes: + """Read a file and return the content as bytes.""" + with open(file=filename, encoding="utf-8") as f: + return f.read() + + def _write_file(self, filename: str, content: bytes) -> None: + """Write a file with the given content.""" + write_file(filename, content) + @authenticated @bind_config - async def post(self, configuration=None): - # Atomic write + async def post(self, configuration: str | None = None): + loop = asyncio.get_running_loop() config_file = settings.rel_path(configuration) - with open(file=config_file, mode="wb") as f: - f.write(self.request.body) - + await loop.run_in_executor( + None, self._write_file, config_file, self.request.body + ) + # Ensure the StorageJSON is updated as well await async_run_system_command( [*DASHBOARD_COMMAND, "compile", "--only-generate", config_file] ) diff --git a/tests/dashboard/util/test_file.py b/tests/dashboard/util/test_file.py index fd4860dbb..89e6b9708 100644 --- a/tests/dashboard/util/test_file.py +++ b/tests/dashboard/util/test_file.py @@ -5,7 +5,7 @@ from unittest.mock import patch import py import pytest -from esphome.dashboard.util.file import write_utf8_file +from esphome.dashboard.util.file import write_file, write_utf8_file def test_write_utf8_file(tmp_path: Path) -> None: @@ -16,6 +16,11 @@ def test_write_utf8_file(tmp_path: Path) -> None: write_utf8_file(Path("/not-writable"), "bar") +def test_write_file(tmp_path: Path) -> None: + write_file(tmp_path.joinpath("foo.txt"), b"foo") + assert tmp_path.joinpath("foo.txt").read_text() == "foo" + + def test_write_utf8_file_fails_at_rename( tmpdir: py.path.local, caplog: pytest.LogCaptureFixture ) -> None: From d0baa20728396a8fb368d5bad35bb37c7e04dc6b Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 18 Nov 2023 09:20:57 -0600 Subject: [PATCH 3/4] more io --- esphome/dashboard/settings.py | 5 +++-- esphome/dashboard/web_server.py | 27 ++++++++++++++++----------- 2 files changed, 19 insertions(+), 13 deletions(-) diff --git a/esphome/dashboard/settings.py b/esphome/dashboard/settings.py index 76633e1bf..505131cba 100644 --- a/esphome/dashboard/settings.py +++ b/esphome/dashboard/settings.py @@ -3,7 +3,7 @@ from __future__ import annotations import hmac import os from pathlib import Path - +from typing import Any from esphome.core import CORE from esphome.helpers import get_bool_env @@ -69,7 +69,8 @@ class DashboardSettings: # Compare password in constant running time (to prevent timing attacks) return hmac.compare_digest(self.password_hash, password_hash(password)) - def rel_path(self, *args): + def rel_path(self, *args: Any) -> str: + """Return a path relative to the ESPHome config folder.""" joined_path = os.path.join(self.config_dir, *args) # Raises ValueError if not relative to ESPHome config folder Path(joined_path).resolve().relative_to(self.absolute_config_dir) diff --git a/esphome/dashboard/web_server.py b/esphome/dashboard/web_server.py index 93d836d76..8901da095 100644 --- a/esphome/dashboard/web_server.py +++ b/esphome/dashboard/web_server.py @@ -525,9 +525,19 @@ class DownloadListRequestHandler(BaseHandler): class DownloadBinaryRequestHandler(BaseHandler): + def _load_file(self, path: str, compressed: bool) -> bytes: + """Load a file from disk and compress it if requested.""" + with open(path, "rb") as f: + data = f.read() + if compressed: + return gzip.compress(data, 9) + return data + @authenticated @bind_config - async def get(self, configuration=None): + async def get(self, configuration: str | None = None): + """Download a binary file.""" + loop = asyncio.get_running_loop() compressed = self.get_argument("compressed", "0") == "1" storage_path = ext_storage_path(configuration) @@ -584,11 +594,8 @@ class DownloadBinaryRequestHandler(BaseHandler): self.send_error(404) return - with open(path, "rb") as f: - data = f.read() - if compressed: - data = gzip.compress(data, 9) - self.write(data) + data = await loop.run_in_executor(None, self._load_file, path, compressed) + self.write(data) self.finish() @@ -748,13 +755,10 @@ class EditRequestHandler(BaseHandler): @authenticated @bind_config async def get(self, configuration: str | None = None): + """Get the content of a file.""" loop = asyncio.get_running_loop() filename = settings.rel_path(configuration) - try: - content = await loop.run_in_executor(None, self._read_file, filename) - except OSError: - self.send_error(404) - return + content = await loop.run_in_executor(None, self._read_file, filename) self.write(content) def _read_file(self, filename: str) -> bytes: @@ -769,6 +773,7 @@ class EditRequestHandler(BaseHandler): @authenticated @bind_config async def post(self, configuration: str | None = None): + """Write the content of a file.""" loop = asyncio.get_running_loop() config_file = settings.rel_path(configuration) await loop.run_in_executor( From 36a5ae1dfb5d0580d8a8202f852ed3088868f0dc Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sat, 18 Nov 2023 09:21:03 -0600 Subject: [PATCH 4/4] more io --- esphome/dashboard/settings.py | 1 + 1 file changed, 1 insertion(+) diff --git a/esphome/dashboard/settings.py b/esphome/dashboard/settings.py index 505131cba..61718298d 100644 --- a/esphome/dashboard/settings.py +++ b/esphome/dashboard/settings.py @@ -4,6 +4,7 @@ import hmac import os from pathlib import Path from typing import Any + from esphome.core import CORE from esphome.helpers import get_bool_env