[store_yaml] Scan tree scalars for leaks, guard key swaps, boot race, OOM check

esphome/components/api/api_connection.cpp

@@ -1218,7 +1218,14 @@ void APIConnection::on_get_yaml_request() {
   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);
+  // Exceptions are disabled, so allocation failure must be checked here. On
+  // failure the request is dropped without a reply; the client times out and
+  // a retry may succeed once the heap recovers.
+  this->store_yaml_chunk_buf_.reset(new (std::nothrow) uint8_t[STORE_YAML_CHUNK_SIZE]);
+  if (!this->store_yaml_chunk_buf_) {
+    ESP_LOGW(TAG, "GetYaml: buffer allocation failed");
+    return;
+  }
 #endif
   // All responses go through the loop-driven retry below, so a full TX
   // buffer at request time can't strand the client without a terminal frame.
@@ -1228,10 +1235,14 @@ void APIConnection::on_get_yaml_request() {
 
 // Caller guarantees: store_yaml_pos_ != SIZE_MAX (a request is in flight).
 void APIConnection::try_send_store_yaml_() {
-  // Every component's setup() completes before the app loop services API
-  // messages, and codegen always embeds a non-empty blob, so the component
-  // is present and total > 0 whenever a request is serviced.
+  // A client connecting while later components are still setting up (the app
+  // loop runs for already-initialized components during setup) can request
+  // YAML before store_yaml's setup() registered the component. Leave the
+  // request pending; this is retried from loop() until the component appears.
   auto *comp = store_yaml::global_store_yaml;
+  if (comp == nullptr)
+    return;
+  // Codegen always embeds a non-empty blob, so total > 0 here.
   const size_t total = comp->get_size();
 
 #ifdef USE_ESP8266

esphome/components/store_yaml/__init__.py

@@ -166,30 +166,58 @@ def _iter_sensitive_values(node: object) -> Generator[tuple[tuple[str, ...], str
             yield path, str(value)
 
 
+def _iter_keys(
+    node: object, path: tuple[str, ...] = ()
+) -> Generator[tuple[tuple[str, ...], str]]:
+    """Yield (config_path, key) for every mapping key in a config tree."""
+    if isinstance(node, dict):
+        for key, value in node.items():
+            yield path, str(key)
+            yield from _iter_keys(value, (*path, str(key)))
+    elif isinstance(node, (list, tuple)):
+        for item in node:
+            yield from _iter_keys(item, path)
+
+
 @dataclass
 class _SensitiveValue:
     secret_name: str
     config_path: str  # dotted path, for warnings (never log the value itself)
+    # Last path segments the value is sensitive at (e.g. {"password"}), used to
+    # tell the value's own occurrences apart from unrelated collisions.
+    sensitive_keys: set[str]
 
 
-def _warn_sensitive_collisions(sensitive: dict[str, _SensitiveValue]) -> None:
+def _warn_sensitive_collisions(
+    sensitive: dict[str, _SensitiveValue], trees: dict[str, object]
+) -> 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."""
+    unrelated scalars too — warn so the trap is documented, not silent.
+
+    Walks the parse trees that are actually dumped, not CORE.config, so the
+    warning matches what lands in the blob. A scalar under the key the value
+    is sensitive at is its own occurrence, not a collision; a swapped
+    `substitutions:` definition keeps `${...}` references working, so those
+    are expected as well.
+    """
     if not sensitive:
         return
-    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:
+    for rel, tree in trees.items():
+        for path, value in _iter_scalars(tree):
+            if (info := sensitive.get(str(value))) is None:
+                continue
+            if path and (path[0] == "substitutions" or path[-1] in info.sensitive_keys):
+                continue
             _LOGGER.warning(
                 "store_yaml: the sensitive value at %s also matches the scalar "
-                "at %s; the recovered config will reference !secret %s there too",
+                "at %s in %s; the recovered config will reference !secret %s "
+                "there too",
                 info.config_path,
                 ".".join(path),
+                rel,
                 info.secret_name,
             )
 
@@ -205,7 +233,11 @@ def _collect_sensitive_values() -> dict[str, _SensitiveValue]:
     used = yaml_util.registered_secret_names()
     result: dict[str, _SensitiveValue] = {}
     for path, value in _iter_sensitive_values(CORE.config):
-        if not value or value in result:
+        if not value:
+            continue
+        if (existing := result.get(value)) is not None:
+            if path:
+                existing.sensitive_keys.add(path[-1])
             continue
         name = yaml_util.is_secret(value)
         if name is None:
@@ -216,7 +248,9 @@ def _collect_sensitive_values() -> dict[str, _SensitiveValue]:
                 name = f"{base}_{counter}"
                 counter += 1
         used.add(name)
-        result[value] = _SensitiveValue(name, ".".join(path))
+        result[value] = _SensitiveValue(
+            name, ".".join(path), {path[-1]} if path else set()
+        )
     return result
 
 
@@ -280,16 +314,35 @@ 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] = {}
+    trees = {
+        rel: yaml_util.load_yaml(path, clear_secrets=False)
+        for rel, path in entries
+        if rel not in secret_rels
+    }
+
+    _warn_sensitive_collisions(sensitive, trees)
+
+    # The dumper routes mapping keys through the same value-keyed swap, so a
+    # sensitive value equal to a key would be rewritten in key position and
+    # silently corrupt the recovered structure — fail the build instead.
+    key_hits = [
+        f"{info.config_path} (as the mapping key at {'.'.join((*path, key))} in {rel})"
+        for rel, tree in trees.items()
+        for path, key in _iter_keys(tree)
+        if (info := sensitive.get(key)) is not None
+    ]
+    if key_hits:
+        raise EsphomeError(
+            "store_yaml: sensitive value(s) are also used as mapping keys: "
+            f"{', '.join(key_hits)}. The redaction swap would rewrite the key "
+            "and corrupt the recovered config. Change the value, or set "
+            "`include_secrets: true` to embed secrets deliberately."
+        )
+
     registered = {value: info.secret_name for value, info in sensitive.items()}
     with yaml_util.secret_values_registered(registered):
-        for rel, path in entries:
-            if rel in secret_rels:
-                continue
-            tree = yaml_util.load_yaml(path, clear_secrets=False)
-            texts[rel] = yaml_util.dump(tree)
+        texts = {rel: yaml_util.dump(tree) for rel, tree in trees.items()}
 
     skeleton_keys: set[str] = set()
     for text in texts.values():
@@ -305,26 +358,34 @@ def _generate_redacted_files(
         if yaml_util.is_secret(value) is None and info.secret_name not in skeleton_keys
     ]
     if leaked:
+        remote = _remote_package_descriptions()
         raise EsphomeError(
             "store_yaml: could not redact the sensitive value(s) of "
-            f"{', '.join(leaked)} (built via substitutions?). Reference them "
-            "with `!secret` in the YAML, or set `include_secrets: true` to "
-            "embed secrets deliberately."
+            f"{', '.join(leaked)}. The value was not found in any captured "
+            "file; it may be composed via substitutions, set on the command "
+            "line with -s, or defined inside a remote package"
+            + (f" ({', '.join(remote)})" if remote else "")
+            + ". Reference it with `!secret` in the YAML, or set "
+            "`include_secrets: true` to 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
-    ]
+    # swapped there, so scan every scalar for a strict substring match and
+    # fail the build on any hit. Scanning tree scalars (not serialized text)
+    # means key names, tags, and generated `!secret` references can never
+    # false-positive; a short value inside an unrelated longer scalar (an
+    # SSID of "esp32" inside "esp32dev") still can, but shipping a
+    # promised-redacted secret is the worse failure, so fail closed.
+    embedded = []
+    for rel, tree in trees.items():
+        for path, scalar in _iter_scalars(tree):
+            text = str(scalar)
+            embedded.extend(
+                f"{info.config_path} (inside {'.'.join(path)} in {rel})"
+                for value, info in sensitive.items()
+                if value in text and value != text
+            )
     if embedded:
         raise EsphomeError(
             "store_yaml: sensitive value(s) appear embedded inside larger "
@@ -398,9 +459,14 @@ def unpack_envelope(blob: bytes) -> dict[str, bytes]:
     (count,) = struct.unpack("<I", take(4))
     for _ in range(count):
         (path_len,) = struct.unpack("<H", take(2))
-        path = take(path_len).decode("utf-8")
+        try:
+            path = take(path_len).decode("utf-8")
+        except UnicodeDecodeError as err:
+            raise EsphomeError(f"envelope path is not valid UTF-8: {err}") from err
         if path.startswith(("/", "\\")) or (len(path) >= 2 and path[1] == ":"):
             raise EsphomeError(f"envelope contains non-relative path: {path}")
+        if path in files:
+            raise EsphomeError(f"envelope contains duplicate path: {path}")
         (content_len,) = struct.unpack("<I", take(4))
         files[path] = take(content_len)
     if pos != len(blob):

script/analyze_component_buses.py

@@ -94,6 +94,7 @@ ISOLATED_COMPONENTS = {
     "modbus_controller": "Defines multiple modbus buses for testing client/server functionality - conflicts with package modbus bus",
     "neopixelbus": "RMT type conflict with ESP32 Arduino/ESP-IDF headers (enum vs struct rmt_channel_t)",
     "packages": "cannot merge packages",
+    "store_yaml": "Embeds the whole merged config in firmware; grouping would make the blob and its secret redaction depend on every grouped component's test config",
     "tinyusb": "Conflicts with usb_host component - cannot be used together",
     "usb_cdc_acm": "Depends on tinyusb which conflicts with usb_host",
 }

tests/unit_tests/components/test_store_yaml.py

@@ -403,6 +403,31 @@ def test_unpack_envelope_rejects_trailing_bytes() -> None:
         unpack_envelope(blob + b"\x00")
 
 
+def test_unpack_envelope_rejects_invalid_utf8_path() -> None:
+    """A corrupted envelope raises EsphomeError, never a bare UnicodeDecodeError."""
+    import struct
+
+    blob = (
+        b"EHY1"
+        + struct.pack("<I", 1)
+        + struct.pack("<H", 2)
+        + b"\xff\xfe"
+        + struct.pack("<I", 0)
+    )
+    with pytest.raises(EsphomeError, match="UTF-8"):
+        unpack_envelope(blob)
+
+
+def test_unpack_envelope_rejects_duplicate_paths() -> None:
+    """A tampered envelope with duplicate paths must not silently drop data."""
+    import struct
+
+    entry = struct.pack("<H", 6) + b"a.yaml" + struct.pack("<I", 1) + b"x"
+    blob = b"EHY1" + struct.pack("<I", 2) + entry + entry
+    with pytest.raises(EsphomeError, match="duplicate"):
+        unpack_envelope(blob)
+
+
 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"):
@@ -439,18 +464,54 @@ 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."""
+    value-keyed swap; a warning documents the trap. The value's own
+    occurrence (under its sensitive key) does not warn."""
     (project / "wifi.yaml").write_text("password: esp32\nplatform: esp32\n")
-    CORE.config = {
-        "wifi": [{"password": SensitiveStr("esp32")}],
-        "sensor": [{"platform": "esp32"}],
-    }
+    CORE.config = {"wifi": [{"password": SensitiveStr("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
+    assert "also matches the scalar at platform in wifi.yaml" in caplog.text
+    assert "at password in wifi.yaml" not in caplog.text
+
+
+def test_redacted_no_warning_for_substitution_definition(
+    project: Path, caplog: pytest.LogCaptureFixture
+) -> None:
+    """A swapped `substitutions:` definition keeps `${...}` working in the
+    recovered config, so it is expected and does not warn."""
+    (project / "wifi.yaml").write_text(
+        "substitutions:\n  wifi_password: hunter2\nwifi:\n  password: ${wifi_password}\n"
+    )
+    CORE.config = {"wifi": [{"password": SensitiveStr("hunter2")}]}
+    discovered = _sources(project, "wifi.yaml")
+    _gather_redacted(discovered)
+    assert "also matches" not in caplog.text
+
+
+def test_redacted_sensitive_value_as_mapping_key_fails_build(project: Path) -> None:
+    """A sensitive value equal to a mapping key would be swapped in key
+    position and corrupt the recovered structure; the build fails instead."""
+    (project / "wifi.yaml").write_text("password: password\n")
+    CORE.config = {"ota": [{"password": SensitiveStr("password")}]}
+    discovered = _sources(project, "wifi.yaml")
+    with pytest.raises(EsphomeError, match="mapping key"):
+        _gather_redacted(discovered)
+
+
+def test_redacted_key_names_do_not_false_positive_embedded_scan(
+    project: Path,
+) -> None:
+    """The embedded scan runs on tree scalars, not serialized text, so a
+    sensitive value that is a substring of a key name (or of the generated
+    `!secret` reference text) does not fail the build."""
+    (project / "wifi.yaml").write_text("password: word\n")
+    CORE.config = {"wifi": [{"password": SensitiveStr("word")}]}
+    discovered = _sources(project, "wifi.yaml")
+    files = _gather_redacted(discovered)
+    assert files["wifi.yaml"] == b"password: !secret 'wifi_password'\n"
 
 
 # ---------------------------------------------------------------------------