diff --git a/esphome/bundle.py b/esphome/bundle.py index dcaea03646..7045e850c7 100644 --- a/esphome/bundle.py +++ b/esphome/bundle.py @@ -29,6 +29,7 @@ from esphome.const import ( CONF_TYPE, ) from esphome.core import CORE, EsphomeError +from esphome.util import filter_yaml_files _LOGGER = logging.getLogger(__name__) @@ -128,6 +129,9 @@ class BundleData: """Files components asked to include, keyed under DOMAIN in CORE.data.""" extra_files: list[Path] = field(default_factory=list) + # Directories whose YAML files are scanned for !secret references but + # never bundled, e.g. git package checkouts the builder re-fetches. + secret_scan_dirs: set[Path] = field(default_factory=set) # Original config dir parsed from an extracted bundle's manifest.json, # kept in the path flavor of the machine the bundle was created on. # The checked flag makes the manifest lookup happen at most once per run; @@ -155,6 +159,30 @@ def add_bundle_file(path: Path) -> None: _get_data().extra_files.append(CORE.relative_config_path(path)) +def add_secret_scan_dir(path: Path) -> None: + """Register a directory to scan for ``!secret`` references when bundling. + + The directory's files are not added to the bundle. Components call this + for YAML the build consumes without bundling it — such as git-fetched + packages, which the builder re-fetches — so the secrets those files + reference are still shipped in the filtered secrets file. + + A relative path is taken as relative to the config directory. + """ + if not path.is_absolute(): + path = CORE.relative_config_path(path) + _get_data().secret_scan_dirs.add(path) + + +def _secret_scan_yaml_files() -> list[Path]: + """Return the YAML files inside registered secret-scan directories.""" + return filter_yaml_files( + f + for scan_dir in _get_data().secret_scan_dirs + for f in yaml_util.find_files(scan_dir, "*") + ) + + # Windows paths start with a drive letter or contain backslashes; POSIX # paths do neither in practice, so this is how the flavor of a recorded # path string is recognized on any host. @@ -310,6 +338,7 @@ class ConfigBundleCreator: yaml_sources = [ bf.source for bf in files if bf.source.suffix in (".yaml", ".yml") ] + yaml_sources.extend(_secret_scan_yaml_files()) used_secret_keys = _find_used_secret_keys(yaml_sources) filtered_secrets = self._build_filtered_secrets(used_secret_keys) @@ -394,6 +423,13 @@ class ConfigBundleCreator: """ discovered = yaml_util.discover_user_yaml_files(self._config_path) self._secrets_paths.update(discovered.secrets) + # A !secret inside a file this re-parse does not reach (for example + # a git-fetched package the builder re-fetches) still resolves + # against the config-dir secrets.yaml at build time, so always + # consider that file; filtering no-ops when no key matches. + default_secrets = self._config_dir / yaml_util.SECRET_YAML + if default_secrets.is_file(): + self._secrets_paths.add(default_secrets.resolve()) config_resolved = self._config_path.resolve() for fpath in discovered.files: if fpath == config_resolved: diff --git a/esphome/components/packages/__init__.py b/esphome/components/packages/__init__.py index 6cb9d5f03a..4d1814ac7a 100644 --- a/esphome/components/packages/__init__.py +++ b/esphome/components/packages/__init__.py @@ -201,6 +201,14 @@ def _process_remote_package(config: dict[str, Any]) -> dict[str, Any]: if base_path := config.get(CONF_PATH): repo_dir = repo_dir / base_path + # Deferred import: keeps esphome.bundle off the device builder's + # startup path, since packages is loaded on every config parse. + from esphome.bundle import add_secret_scan_dir + + # Register the path-narrowed dir, not repo_root, so example configs + # elsewhere in the repo do not widen the shipped secrets. + add_secret_scan_dir(repo_dir) + for file in config[CONF_FILES]: if isinstance(file, str): files.append({CONF_PATH: file, CONF_VARS: {}}) diff --git a/esphome/util.py b/esphome/util.py index f7d33bd2a9..ed95ea24d2 100644 --- a/esphome/util.py +++ b/esphome/util.py @@ -1,5 +1,5 @@ import collections -from collections.abc import Callable +from collections.abc import Callable, Iterable from dataclasses import dataclass import io import logging @@ -356,7 +356,7 @@ def list_yaml_files(configs: list[str | Path]) -> list[Path]: return sorted(files) -def filter_yaml_files(files: list[Path]) -> list[Path]: +def filter_yaml_files(files: Iterable[Path]) -> list[Path]: return [ f for f in files diff --git a/esphome/yaml_util.py b/esphome/yaml_util.py index 833d6f1dbf..03d1e81073 100644 --- a/esphome/yaml_util.py +++ b/esphome/yaml_util.py @@ -1,6 +1,6 @@ from __future__ import annotations -from collections.abc import Callable, Generator +from collections.abc import Callable, Generator, Iterator from contextlib import contextmanager, suppress from dataclasses import dataclass, field import functools @@ -791,12 +791,12 @@ class ESPHomeLoaderMixin: @_add_data_ref def construct_include_dir_list(self, node: yaml.Node) -> list[dict[str, Any]]: - files = filter_yaml_files(_find_files(self._rel_path(node.value), "*.yaml")) + files = filter_yaml_files(find_files(self._rel_path(node.value), "*.yaml")) return [self.yaml_loader(f) for f in files] @_add_data_ref def construct_include_dir_merge_list(self, node: yaml.Node) -> list[dict[str, Any]]: - files = filter_yaml_files(_find_files(self._rel_path(node.value), "*.yaml")) + files = filter_yaml_files(find_files(self._rel_path(node.value), "*.yaml")) merged_list = [] for fname in files: loaded_yaml = self.yaml_loader(fname) @@ -808,7 +808,7 @@ class ESPHomeLoaderMixin: def construct_include_dir_named( self, node: yaml.Node ) -> OrderedDict[str, dict[str, Any]]: - files = filter_yaml_files(_find_files(self._rel_path(node.value), "*.yaml")) + files = filter_yaml_files(find_files(self._rel_path(node.value), "*.yaml")) mapping = OrderedDict() for fname in files: filename = fname.stem @@ -819,7 +819,7 @@ class ESPHomeLoaderMixin: def construct_include_dir_merge_named( self, node: yaml.Node ) -> OrderedDict[str, dict[str, Any]]: - files = filter_yaml_files(_find_files(self._rel_path(node.value), "*.yaml")) + files = filter_yaml_files(find_files(self._rel_path(node.value), "*.yaml")) mapping = OrderedDict() for fname in files: loaded_yaml = self.yaml_loader(fname) @@ -1015,8 +1015,8 @@ def _is_file_valid(name: str) -> bool: return not name.startswith(".") -def _find_files(directory: Path, pattern): - """Recursively load files in a directory.""" +def find_files(directory: Path, pattern: str) -> Iterator[Path]: + """Recursively find files in a directory matching *pattern*, skipping hidden entries.""" for root, dirs, files in os.walk(directory): dirs[:] = [d for d in dirs if _is_file_valid(d)] for f in files: diff --git a/tests/component_tests/packages/test_packages.py b/tests/component_tests/packages/test_packages.py index 39bffd31b7..418cab5ea1 100644 --- a/tests/component_tests/packages/test_packages.py +++ b/tests/component_tests/packages/test_packages.py @@ -7,6 +7,7 @@ from unittest.mock import MagicMock, patch import pytest +from esphome import bundle from esphome.components.packages import ( CONFIG_SCHEMA, _substitute_package_definition, @@ -1694,3 +1695,34 @@ def test_resolve_packages_does_not_apply_extend_remove() -> None: # over the package value during merge), and the marker is not # resolved by this wrapper. assert isinstance(result[CONF_WIFI], Remove) + + +@patch("esphome.git.clone_or_update") +def test_remote_package_registers_checkout_for_secret_scan( + mock_clone_or_update, tmp_path: Path +) -> None: + """Loading a remote package registers its path-narrowed checkout dir + as a bundle secret-scan dir (issue 18023).""" + repo_root = tmp_path / "repo" + package_dir = repo_root / "packages" + package_dir.mkdir(parents=True) + (package_dir / "base.yml").write_text( + f"sensor:\n - platform: {TEST_SENSOR_PLATFORM_1}\n name: {TEST_SENSOR_NAME_1}\n" + ) + mock_clone_or_update.return_value = (repo_root, None) + + config = { + CONF_PACKAGES: { + "package1": { + CONF_URL: "https://github.com/esphome/non-existant-repo", + CONF_REF: "main", + CONF_PATH: "packages", + CONF_FILES: ["base.yml"], + CONF_REFRESH: "1d", + } + } + } + packages_pass(config) + + assert package_dir in bundle._get_data().secret_scan_dirs + assert repo_root not in bundle._get_data().secret_scan_dirs diff --git a/tests/unit_tests/test_bundle.py b/tests/unit_tests/test_bundle.py index 5c71b72d86..785db4086c 100644 --- a/tests/unit_tests/test_bundle.py +++ b/tests/unit_tests/test_bundle.py @@ -23,6 +23,7 @@ from esphome.bundle import ( _default_target_dir, _find_used_secret_keys, add_bundle_file, + add_secret_scan_dir, extract_bundle, is_bundle_path, prepare_bundle_for_compile, @@ -1651,6 +1652,44 @@ def test_create_bundle_filters_secrets_quoted(tmp_path: Path) -> None: assert "unused" not in secrets_data +def test_create_bundle_scans_remote_package_files_for_secrets(tmp_path: Path) -> None: + """Secrets referenced only by git-fetched package files must be shipped + in the filtered secrets.yaml (regression test for issue 18023).""" + config_dir = _setup_config_dir(tmp_path) + + secrets = config_dir / "secrets.yaml" + secrets.write_text("ota_password: hunter2\nunused: should_not_appear\n") + + # Simulate a git-fetched package checkout referencing a secret + repo_dir = config_dir / ".esphome" / "packages" / "6bcd6aa8" + package_dir = repo_dir / "packages" + package_dir.mkdir(parents=True) + (package_dir / "base.yml").write_text( + "ota:\n - platform: esphome\n password: !secret ota_password\n" + ) + # References inside hidden directories such as .git must not be scanned + hidden_dir = repo_dir / ".git" + hidden_dir.mkdir() + (hidden_dir / "leak.yaml").write_text("password: !secret unused\n") + add_secret_scan_dir(repo_dir) + + creator = ConfigBundleCreator({}) + result = creator.create_bundle() + + assert result.manifest[ManifestKey.HAS_SECRETS] is True + + buf = io.BytesIO(result.data) + with tarfile.open(fileobj=buf, mode="r:gz") as tar: + secrets_data = tar.extractfile("secrets.yaml").read().decode() + names = tar.getnames() + + assert "ota_password" in secrets_data + assert "hunter2" in secrets_data + assert "unused" not in secrets_data + # The package checkout itself must not be bundled + assert not any("base.yml" in name for name in names) + + def test_create_bundle_no_secrets(tmp_path: Path) -> None: _setup_config_dir(tmp_path) diff --git a/tests/unit_tests/test_yaml_util.py b/tests/unit_tests/test_yaml_util.py index 7a08ad2eb4..1bb70864a3 100644 --- a/tests/unit_tests/test_yaml_util.py +++ b/tests/unit_tests/test_yaml_util.py @@ -282,8 +282,54 @@ test: !include_dir_named test_dir assert ".hidden_dir" not in actual["test"] +def test_include_dir_list(tmp_path: Path) -> None: + """!include_dir_list loads every .yaml file in the directory as a list.""" + test_dir = tmp_path / "test_dir" + test_dir.mkdir() + (test_dir / "a.yaml").write_text("key: value_a") + (test_dir / "b.yaml").write_text("key: value_b") + + test_yaml = tmp_path / "test.yaml" + test_yaml.write_text("test: !include_dir_list test_dir\n") + + actual = yaml_util.load_yaml(test_yaml) + + assert len(actual["test"]) == 2 + assert {entry["key"] for entry in actual["test"]} == {"value_a", "value_b"} + + +def test_include_dir_merge_list(tmp_path: Path) -> None: + """!include_dir_merge_list concatenates the lists from every .yaml file.""" + test_dir = tmp_path / "test_dir" + test_dir.mkdir() + (test_dir / "a.yaml").write_text("- item_a1\n- item_a2\n") + (test_dir / "b.yaml").write_text("- item_b1\n") + + test_yaml = tmp_path / "test.yaml" + test_yaml.write_text("test: !include_dir_merge_list test_dir\n") + + actual = yaml_util.load_yaml(test_yaml) + + assert sorted(actual["test"]) == ["item_a1", "item_a2", "item_b1"] + + +def test_include_dir_merge_named(tmp_path: Path) -> None: + """!include_dir_merge_named merges the mappings from every .yaml file.""" + test_dir = tmp_path / "test_dir" + test_dir.mkdir() + (test_dir / "a.yaml").write_text("key_a: value_a") + (test_dir / "b.yaml").write_text("key_b: value_b") + + test_yaml = tmp_path / "test.yaml" + test_yaml.write_text("test: !include_dir_merge_named test_dir\n") + + actual = yaml_util.load_yaml(test_yaml) + + assert actual["test"] == {"key_a": "value_a", "key_b": "value_b"} + + def test_find_files_recursive(fixture_path: Path, tmp_path: Path) -> None: - """Test that _find_files works recursively through include_dir_named.""" + """Test that find_files works recursively through include_dir_named.""" # Copy fixture directory to temporary location src_dir = fixture_path / "yaml_util" dst_dir = tmp_path / "yaml_util"