[store_yaml] Address review: chunking, re-request no-op, capture gaps, leak scan, tests

esphome/bundle.py

@@ -107,7 +107,13 @@ def _find_used_secret_keys(yaml_files: list[Path]) -> set[str]:
     for fpath in yaml_files:
         try:
             text = fpath.read_text(encoding="utf-8")
-        except (OSError, UnicodeDecodeError):
+        except (OSError, UnicodeDecodeError) as err:
+            _LOGGER.warning(
+                "Could not scan %s for !secret references (%s); the bundled "
+                "secret set may be incomplete",
+                fpath,
+                err,
+            )
             continue
         keys |= yaml_util.find_secret_references(text)
     return keys
@@ -383,6 +389,18 @@ class ConfigBundleCreator:
         must ship every candidate so the remote build can pick any one.
         """
         discovered = yaml_util.discover_user_yaml_files(self._config_path)
+        if discovered.load_errors:
+            _LOGGER.warning(
+                "Bundle may be incomplete; could not load all configuration files: %s",
+                "; ".join(discovered.load_errors),
+            )
+        if discovered.unresolved:
+            _LOGGER.warning(
+                "Bundle may be incomplete; %d !include path(s) use "
+                "substitutions and cannot be captured: %s",
+                len(discovered.unresolved),
+                ", ".join(discovered.unresolved),
+            )
         self._secrets_paths.update(discovered.secrets)
         config_resolved = self._config_path.resolve()
         for fpath in discovered.files:

esphome/components/api/api.proto

@@ -2764,6 +2764,10 @@ message BluetoothSetConnectionParamsResponse {
 // Embed the user's YAML in firmware and stream it back over the API so a lost
 // config can be recovered from a running device. The device only stores the
 // compressed bytes; decompression happens client-side.
+//
+// A GetYamlRequest received while a transfer is already streaming on the same
+// connection is ignored; the in-flight transfer continues undisturbed. To
+// restart a transfer, reconnect.
 message GetYamlRequest {
   option (id) = 149;
   option (source) = SOURCE_CLIENT;

esphome/components/api/api_connection.cpp

@@ -1202,20 +1202,24 @@ void APIConnection::on_camera_image_request(const CameraImageRequest &msg) {
 #endif
 
 #ifdef USE_STORE_YAML
-// Chunk size per GetYamlResponse. Small enough to leave room for the protobuf frame
-// inside the 65535-byte API limit and friendly to TCP MSS.
-static constexpr size_t STORE_YAML_CHUNK_SIZE = 512;
 #ifdef USE_ESP8266
 // On ESP8266 the blob lives in instruction flash and can't be read directly, so
-// each chunk is bounced through this static buffer via progmem_memcpy. Shared
-// across connections is safe because the API loop is single-threaded and each
-// chunk is filled and consumed atomically inside one `try_send_store_yaml_`
-// iteration. Every other platform sends straight from the blob, zero-copy.
-// NOLINTNEXTLINE(cppcoreguidelines-avoid-non-const-global-variables)
-static uint8_t store_yaml_chunk_buf[STORE_YAML_CHUNK_SIZE];
+// each chunk is bounced through a heap buffer via progmem_memcpy. The buffer
+// exists only while a transfer is in flight; retrieval is rare, so no RAM is
+// held for the firmware's lifetime. Every other platform sends straight from
+// the blob, zero-copy, in MTU-sized chunks.
+static constexpr size_t STORE_YAML_CHUNK_SIZE = 512;
 #endif
 
 void APIConnection::on_get_yaml_request() {
+  // A re-request while a transfer is in flight is ignored; see the
+  // GetYamlRequest comment in api.proto. A client that wants to restart
+  // must reconnect.
+  if (this->store_yaml_pos_ != std::numeric_limits<size_t>::max())
+    return;
+#ifdef USE_ESP8266
+  this->store_yaml_chunk_buf_ = std::make_unique<uint8_t[]>(STORE_YAML_CHUNK_SIZE);
+#endif
   // All responses — including the single data-less done=true frame for a
   // missing/empty blob — go through the loop-driven retry below, so a full
   // TX buffer at request time can't strand the client without a terminal frame.
@@ -1230,6 +1234,12 @@ void APIConnection::try_send_store_yaml_() {
   // treat that like an empty blob and send just the terminal frame.
   const size_t total = comp == nullptr ? 0 : comp->get_size();
 
+#ifdef USE_ESP8266
+  const size_t chunk_size = STORE_YAML_CHUNK_SIZE;
+#else
+  const size_t chunk_size = MAX_BATCH_PACKET_SIZE;
+#endif
+
   // Camera-style streaming: advance the position only after a successful send,
   // so a WOULD_BLOCK simply retries the same chunk on the next loop iteration.
   while (true) {
@@ -1237,13 +1247,13 @@ void APIConnection::try_send_store_yaml_() {
       return;
 
     const size_t remaining = total - this->store_yaml_pos_;
-    const size_t to_send = std::min(remaining, STORE_YAML_CHUNK_SIZE);
+    const size_t to_send = std::min(remaining, chunk_size);
 
     GetYamlResponse resp;
     if (to_send != 0) {
 #ifdef USE_ESP8266
-      progmem_memcpy(store_yaml_chunk_buf, comp->get_data() + this->store_yaml_pos_, to_send);
-      resp.set_data(store_yaml_chunk_buf, to_send);
+      progmem_memcpy(this->store_yaml_chunk_buf_.get(), comp->get_data() + this->store_yaml_pos_, to_send);
+      resp.set_data(this->store_yaml_chunk_buf_.get(), to_send);
 #else
       resp.set_data(comp->get_data() + this->store_yaml_pos_, to_send);
 #endif
@@ -1268,6 +1278,9 @@ void APIConnection::try_send_store_yaml_() {
 
   // Final response (with done=true) sent successfully.
   this->store_yaml_pos_ = std::numeric_limits<size_t>::max();
+#ifdef USE_ESP8266
+  this->store_yaml_chunk_buf_.reset();
+#endif
 }
 #endif
 

esphome/components/api/api_connection.h

@@ -29,6 +29,7 @@
 
 #include <functional>
 #include <limits>
+#include <memory>
 #include <vector>
 
 namespace esphome {
@@ -406,6 +407,12 @@ class APIConnection final : public APIServerConnectionBase {
   void try_send_store_yaml_();
   // Streaming offset into the PROGMEM blob; max() means "not streaming".
   size_t store_yaml_pos_{std::numeric_limits<size_t>::max()};
+#ifdef USE_ESP8266
+  // Bounce buffer for progmem_memcpy, alive only while a transfer is in
+  // flight; retrieval is rare, so the RAM is not held for the firmware's
+  // lifetime. Freed on the terminal frame or with the connection.
+  std::unique_ptr<uint8_t[]> store_yaml_chunk_buf_;
+#endif
 #endif
 
 #ifdef USE_API_HOMEASSISTANT_STATES

esphome/components/store_yaml/__init__.py

@@ -171,6 +171,40 @@ class _SensitiveValue:
     config_path: str  # dotted path, for warnings (never log the value itself)
 
 
+def _iter_scalars(
+    node: object, path: tuple[str, ...] = ()
+) -> Generator[tuple[tuple[str, ...], object]]:
+    """Yield (config_path, value) for every scalar in a config tree."""
+    if isinstance(node, dict):
+        for key, value in node.items():
+            yield from _iter_scalars(value, (*path, str(key)))
+    elif isinstance(node, (list, tuple)):
+        for item in node:
+            yield from _iter_scalars(item, path)
+    elif isinstance(node, (str, int, float)) and not isinstance(node, bool):
+        yield path, node
+
+
+def _warn_sensitive_collisions(sensitive: dict[str, _SensitiveValue]) -> None:
+    """Redaction is value-keyed: any scalar equal to a sensitive value is
+    rewritten to its `!secret` reference, including unrelated ones (e.g.
+    `platform: esp32` when a password is literally "esp32"). Filling in a
+    different value during recovery would then silently rewrite those
+    unrelated scalars too — warn so the trap is documented, not silent."""
+    for path, value in _iter_scalars(CORE.config):
+        if isinstance(value, yaml_util.SensitiveStr):
+            continue
+        info = sensitive.get(str(value))
+        if info is not None:
+            _LOGGER.warning(
+                "store_yaml: the sensitive value at %s also matches the scalar "
+                "at %s; the recovered config will reference !secret %s there too",
+                info.config_path,
+                ".".join(path),
+                info.secret_name,
+            )
+
+
 def _collect_sensitive_values() -> dict[str, _SensitiveValue]:
     """Map each cv.sensitive value in the validated config to the `!secret`
     name it should be recovered as.
@@ -197,20 +231,68 @@ def _collect_sensitive_values() -> dict[str, _SensitiveValue]:
     return result
 
 
-# Envelope path of the note recording includes that could not be captured.
+# Envelope path of the note recording content that could not be captured.
 UNCAPTURED_NOTE_PATH = "store_yaml_uncaptured.yaml"
 
 
-def _uncaptured_note(unresolved: list[str]) -> tuple[str, bytes]:
-    """Comment-only YAML entry listing includes that could not be captured, so
+def _uncaptured_note(
+    unresolved: list[str], remote_packages: list[str]
+) -> tuple[str, bytes]:
+    """Comment-only YAML entry listing content that could not be captured, so
     a recovered config never silently appears complete. Emitted for both the
     redacted and verbatim paths; user files are never modified to carry it."""
-    text = (
-        "# store_yaml: the following !include paths use substitutions and\n"
-        "# could not be captured; restore these files manually:\n"
-        + "".join(f"#   {inc}\n" for inc in unresolved)
-    )
-    return (UNCAPTURED_NOTE_PATH, text.encode("utf-8"))
+    parts = ["# store_yaml: the following content could not be captured.\n"]
+    if unresolved:
+        parts.append(
+            "# These !include paths use substitutions; restore the files manually:\n"
+            + "".join(f"#   {inc}\n" for inc in unresolved)
+        )
+    if remote_packages:
+        parts.append(
+            "# These packages come from remote sources; re-fetch them to\n"
+            "# complete this config:\n"
+            + "".join(f"#   {pkg}\n" for pkg in remote_packages)
+        )
+    return (UNCAPTURED_NOTE_PATH, "".join(parts).encode("utf-8"))
+
+
+def _find_remote_packages(entries: list[tuple[str, Path]]) -> list[str]:
+    """Describe every `packages:` entry that pulls content from a remote source.
+
+    Remote packages are downloaded during validation, which the fresh parse
+    used for discovery never reaches, so their files cannot be embedded. The
+    entry file still records the source, so the config is re-fetchable; this
+    only makes the gap visible instead of silent.
+    """
+    remote: list[str] = []
+    for _, path in entries:
+        try:
+            tree = yaml_util.load_yaml(path, clear_secrets=False)
+        except EsphomeError:
+            # Discovery already loaded this file once; a failure here would
+            # have been reported as a load_error and failed the build.
+            continue
+        if not isinstance(tree, dict):
+            continue
+        packages = tree.get("packages")
+        if isinstance(packages, dict):
+            candidates = packages.items()
+        elif isinstance(packages, list):
+            candidates = ((None, item) for item in packages)
+        else:
+            continue
+        for name, value in candidates:
+            desc = None
+            if isinstance(value, dict) and "url" in value:
+                url = value.get("url")
+                ref = value.get("ref")
+                desc = f"{url}@{ref}" if ref else str(url)
+            elif isinstance(value, str) and "//" in value:
+                # Shorthand form, e.g. `github://org/repo/file.yaml@main`
+                desc = value
+            if desc is not None:
+                remote.append(f"{name}: {desc}" if name is not None else desc)
+    return remote
 
 
 def _build_secrets_skeleton(keys: set[str]) -> bytes:
@@ -233,6 +315,7 @@ def _generate_redacted_files(
     comments and formatting of the originals are not preserved.
     """
     sensitive = _collect_sensitive_values()
+    _warn_sensitive_collisions(sensitive)
 
     texts: dict[str, str] = {}
     registered = {value: info.secret_name for value, info in sensitive.items()}
@@ -264,6 +347,28 @@ def _generate_redacted_files(
             "embed secrets deliberately."
         )
 
+    # The swap replaces whole scalars only. A sensitive value embedded inside
+    # a larger scalar — a lambda body, a URL like http://user:pw@host — is not
+    # swapped there, so scan the generated output for every sensitive value as
+    # a substring and fail the build on any hit. This can false-positive on
+    # short values that legitimately appear as substrings (an SSID of "esp32"
+    # inside "esp32dev"); shipping a promised-redacted secret is the worse
+    # failure, so fail closed.
+    embedded = [
+        f"{info.config_path} (inside {rel})"
+        for value, info in sensitive.items()
+        for rel, text in texts.items()
+        if value in text
+    ]
+    if embedded:
+        raise EsphomeError(
+            "store_yaml: sensitive value(s) appear embedded inside larger "
+            f"values: {', '.join(embedded)}. Redaction only replaces whole "
+            "scalars, so these would ship unredacted. Move the value into a "
+            "`!secret` referenced on its own, or set `include_secrets: true` "
+            "to embed secrets deliberately."
+        )
+
     skeleton = _build_secrets_skeleton(skeleton_keys)
     result = [
         (rel, skeleton if rel in secret_rels else texts[rel].encode("utf-8"))
@@ -284,7 +389,13 @@ def _pack_envelope(files: list[tuple[str, bytes]]) -> bytes:
     All integers are little-endian.
     """
     parts: list[bytes] = [ENVELOPE_MAGIC, struct.pack("<I", len(files))]
+    seen: set[str] = set()
     for path, content in files:
+        if path in seen:
+            # unpack_envelope builds a dict, so a duplicate would silently
+            # replace the earlier entry — fail the build instead.
+            raise EsphomeError(f"store_yaml: duplicate envelope path: {path}")
+        seen.add(path)
         path_bytes = path.encode("utf-8")
         if len(path_bytes) > 0xFFFF:
             raise EsphomeError(
@@ -299,7 +410,13 @@ def _pack_envelope(files: list[tuple[str, bytes]]) -> bytes:
 
 def unpack_envelope(blob: bytes) -> dict[str, bytes]:
     """Inverse of `_pack_envelope`: the reference decoder for the EHY1 envelope,
-    used by tests and client-side recovery tooling."""
+    used by tests and client-side recovery tooling.
+
+    Absolute and drive-qualified paths are rejected: the packer never emits
+    them, so their presence means a malformed or hostile envelope. Relative
+    paths with ``..`` components are legitimate (the packer emits them for
+    files outside the config root), so callers that write files to disk must
+    still confine the resulting paths to their target directory."""
     if blob[:4] != ENVELOPE_MAGIC:
         raise EsphomeError("envelope must start with EHY1 magic")
     pos = 4
@@ -313,6 +430,8 @@ def unpack_envelope(blob: bytes) -> dict[str, bytes]:
             if pos + path_len > len(blob):
                 raise EsphomeError("truncated envelope")
             path = blob[pos : pos + path_len].decode("utf-8")
+            if path.startswith(("/", "\\")) or (len(path) >= 2 and path[1] == ":"):
+                raise EsphomeError(f"envelope contains non-relative path: {path}")
             pos += path_len
             (content_len,) = struct.unpack_from("<I", blob, pos)
             pos += 4
@@ -341,8 +460,17 @@ async def to_code(config: ConfigType) -> None:
         files = _read_files_verbatim(entries)
     else:
         files = _generate_redacted_files(entries, secret_rels)
-    if discovered.unresolved:
-        files.append(_uncaptured_note(discovered.unresolved))
+    remote_packages = _find_remote_packages(entries)
+    if remote_packages:
+        _LOGGER.warning(
+            "store_yaml: %d package(s) come from remote sources and cannot be "
+            "captured (%s); the embedded recovery data records the source so "
+            "they can be re-fetched",
+            len(remote_packages),
+            ", ".join(remote_packages),
+        )
+    if discovered.unresolved or remote_packages:
+        files.append(_uncaptured_note(discovered.unresolved, remote_packages))
     envelope = _pack_envelope(files)
     compressed = zstd.compress(envelope, level=ZSTD_LEVEL)
 

tests/components/store_yaml/validate.esp32-idf.yaml

@@ -0,0 +1,9 @@
+wifi:
+  ssid: MySSID
+  password: password1
+
+api:
+  encryption:
+    key: AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8=
+
+store_yaml:

tests/unit_tests/components/test_store_yaml.py

@@ -9,8 +9,11 @@ import pytest
 
 from esphome import yaml_util
 from esphome.components.store_yaml import (
+    CONF_ALLOW_UNENCRYPTED,
     SECRETS_SKELETON_HEADER,
     UNCAPTURED_NOTE_PATH,
+    _final_validate,
+    _find_remote_packages,
     _gather_files,
     _generate_redacted_files,
     _pack_envelope,
@@ -18,7 +21,9 @@ from esphome.components.store_yaml import (
     _uncaptured_note,
     unpack_envelope,
 )
+import esphome.config_validation as cv
 from esphome.core import CORE, EsphomeError
+import esphome.final_validate as fv
 from esphome.yaml_util import DiscoveredYamlFiles, SensitiveStr
 
 
@@ -213,18 +218,15 @@ def test_redacted_quoted_inline_value(project: Path, quote: str) -> None:
 
 def test_redacted_swap_is_whole_scalar_and_value_keyed(project: Path) -> None:
     """Every whole scalar equal to the sensitive value is swapped (value-keyed,
-    like `!secret` itself); substrings inside other scalars are never touched.
-    The recovered config stays semantically identical once the secret is filled."""
-    (project / "wifi.yaml").write_text(
-        "platform: esp32\nnote: esp32 is great\npassword: esp32\n"
-    )
+    like `!secret` itself). The recovered config stays semantically identical
+    once the secret is filled."""
+    (project / "wifi.yaml").write_text("platform: esp32\npassword: esp32\n")
     CORE.config = {"wifi": [{"password": SensitiveStr("esp32")}]}
     discovered = _sources(project, "wifi.yaml")
     files = _gather_redacted(discovered)
     text = files["wifi.yaml"].decode()
     assert "password: !secret 'wifi_password'" in text
     assert "platform: !secret 'wifi_password'" in text
-    assert "note: esp32 is great" in text
 
 
 def test_redacted_include_reference_round_trips(project: Path) -> None:
@@ -277,13 +279,53 @@ def test_redacted_accepts_secret_only_values(project: Path) -> None:
 def test_uncaptured_note_lists_missing_includes() -> None:
     """Substitution-pathed includes that can't be captured are recorded in a
     dedicated envelope entry (both modes), not just a compile-time log line."""
-    rel, content = _uncaptured_note(["${board}.yaml"])
+    rel, content = _uncaptured_note(["${board}.yaml"], [])
     assert rel == UNCAPTURED_NOTE_PATH
     text = content.decode()
     assert text.startswith("# store_yaml:")
     assert "#   ${board}.yaml" in text
 
 
+def test_uncaptured_note_lists_remote_packages() -> None:
+    """Remote packages that can't be captured are recorded with their source
+    so the user knows to re-fetch them."""
+    rel, content = _uncaptured_note(
+        [], ["base: https://github.com/org/repo@main", "github://org/repo/file.yaml"]
+    )
+    assert rel == UNCAPTURED_NOTE_PATH
+    text = content.decode()
+    assert "#   base: https://github.com/org/repo@main" in text
+    assert "#   github://org/repo/file.yaml" in text
+
+
+def test_find_remote_packages_detects_url_and_shorthand(project: Path) -> None:
+    """`packages:` entries with a url (dict or shorthand string) are reported;
+    local `!include` packages are not."""
+    (project / "entry.yaml").write_text(
+        "packages:\n"
+        "  base:\n"
+        "    url: https://github.com/org/repo\n"
+        "    ref: main\n"
+        "    files: [common.yaml]\n"
+        "  shorthand: github://org/repo/file.yaml@main\n"
+        "  local: !include wifi.yaml\n"
+        "esphome:\n  name: test\n"
+    )
+    discovered = _sources(project, "entry.yaml", "wifi.yaml")
+    entries, _ = _gather_files(discovered)
+    remote = _find_remote_packages(entries)
+    assert remote == [
+        "base: https://github.com/org/repo@main",
+        "shorthand: github://org/repo/file.yaml@main",
+    ]
+
+
+def test_find_remote_packages_ignores_local_only(project: Path) -> None:
+    discovered = _sources(project, "entry.yaml", "wifi.yaml")
+    entries, _ = _gather_files(discovered)
+    assert _find_remote_packages(entries) == []
+
+
 def test_redacted_skips_empty_sensitive_values(project: Path) -> None:
     """Empty defaults (e.g. mqtt password) are never swapped."""
     (project / "wifi.yaml").write_text("ssid: my_ssid\n")
@@ -362,3 +404,87 @@ def test_unpack_envelope_rejects_trailing_bytes() -> None:
     blob = _pack_envelope([("entry.yaml", b"esphome:\n")])
     with pytest.raises(EsphomeError, match="trailing"):
         unpack_envelope(blob + b"\x00")
+
+
+def test_pack_envelope_rejects_duplicate_paths() -> None:
+    """A duplicate path would silently clobber the earlier entry on unpack."""
+    with pytest.raises(EsphomeError, match="duplicate"):
+        _pack_envelope([("entry.yaml", b"a: 1\n"), ("entry.yaml", b"b: 2\n")])
+
+
+@pytest.mark.parametrize("path", ["/etc/passwd", "\\evil.yaml", "C:/evil.yaml"])
+def test_unpack_envelope_rejects_non_relative_paths(path: str) -> None:
+    """The packer never emits absolute or drive-qualified paths, so their
+    presence means a malformed or hostile envelope."""
+    blob = _pack_envelope([(path, b"boom\n")])
+    with pytest.raises(EsphomeError, match="non-relative"):
+        unpack_envelope(blob)
+
+
+# ---------------------------------------------------------------------------
+# embedded-value leak scan and collision warning
+# ---------------------------------------------------------------------------
+
+
+def test_redacted_embedded_sensitive_value_fails_build(project: Path) -> None:
+    """A sensitive value inside a larger scalar (URL, lambda body) is not
+    swapped by the whole-scalar redaction; the substring scan fails closed."""
+    (project / "wifi.yaml").write_text(
+        "password: my_password\nurl: http://user:my_password@host\n"
+    )
+    CORE.config = {"wifi": [{"password": SensitiveStr("my_password")}]}
+    discovered = _sources(project, "wifi.yaml")
+    with pytest.raises(EsphomeError, match="embedded"):
+        _gather_redacted(discovered)
+
+
+def test_redacted_warns_on_value_collision(
+    project: Path, caplog: pytest.LogCaptureFixture
+) -> None:
+    """An unrelated scalar equal to a sensitive value gets rewritten by the
+    value-keyed swap; a warning documents the trap."""
+    (project / "wifi.yaml").write_text("password: esp32\nplatform: esp32\n")
+    CORE.config = {
+        "wifi": [{"password": SensitiveStr("esp32")}],
+        "sensor": [{"platform": "esp32"}],
+    }
+    discovered = _sources(project, "wifi.yaml")
+    files = _gather_redacted(discovered)
+    assert files["wifi.yaml"] == (
+        b"password: !secret 'wifi_password'\nplatform: !secret 'wifi_password'\n"
+    )
+    assert "also matches the scalar at sensor.platform" in caplog.text
+
+
+# ---------------------------------------------------------------------------
+# _final_validate encryption gate
+# ---------------------------------------------------------------------------
+
+
+def _run_final_validate(full_config: dict, config: dict) -> dict:
+    token = fv.full_config.set(full_config)
+    try:
+        return _final_validate(config)
+    finally:
+        fv.full_config.reset(token)
+
+
+def test_final_validate_accepts_encrypted_api() -> None:
+    config = {CONF_ALLOW_UNENCRYPTED: False}
+    full = {
+        "api": {"encryption": {"key": "AAECAwQFBgcICQoLDA0ODxAREhMUFRYXGBkaGxwdHh8="}}
+    }
+    assert _run_final_validate(full, config) is config
+
+
+def test_final_validate_rejects_unencrypted_api() -> None:
+    with pytest.raises(cv.Invalid, match="requires API encryption"):
+        _run_final_validate({"api": {}}, {CONF_ALLOW_UNENCRYPTED: False})
+
+
+def test_final_validate_allows_unencrypted_with_escape_hatch(
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    config = {CONF_ALLOW_UNENCRYPTED: True}
+    assert _run_final_validate({"api": {}}, config) is config
+    assert "without API encryption" in caplog.text