[core] Ship secrets referenced by remote package files in config bundles (#18053)

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:

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: {}})

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

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):
+def find_files(directory: Path, pattern: str) -> Iterator[Path]:
-    """Recursively load files in a directory."""
+    """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:

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

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)
 

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"