From c625c6b795d8ef5f69d120d787a45492a112a97e Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Fri, 3 Jul 2026 20:57:54 -0500 Subject: [PATCH] [store_yaml] Address review: retry terminal frame, fail on unredactable secrets, note uncaptured includes --- esphome/components/api/api.proto | 2 +- esphome/components/api/api_connection.cpp | 39 ++++++++------- esphome/components/api/api_pb2.cpp | 5 +- esphome/components/store_yaml/__init__.py | 48 +++++++++++++------ .../unit_tests/components/test_store_yaml.py | 39 ++++++++------- 5 files changed, 80 insertions(+), 53 deletions(-) diff --git a/esphome/components/api/api.proto b/esphome/components/api/api.proto index c138140a48..2aad560561 100644 --- a/esphome/components/api/api.proto +++ b/esphome/components/api/api.proto @@ -2758,7 +2758,7 @@ message GetYamlResponse { option (ifdef) = "USE_STORE_YAML"; bytes data = 1 [(force) = true]; - bool done = 2; + bool done = 2 [(force) = true]; // Sent on the first chunk only — the client is expected to cache it. // (firmware bandwidth/flash is expensive; the client has gigabytes.) uint32 total_size = 3; diff --git a/esphome/components/api/api_connection.cpp b/esphome/components/api/api_connection.cpp index 5af7bfffad..2b13e8dcd7 100644 --- a/esphome/components/api/api_connection.cpp +++ b/esphome/components/api/api_connection.cpp @@ -1180,28 +1180,23 @@ static uint8_t store_yaml_chunk_buf[STORE_YAML_CHUNK_SIZE]; #endif void APIConnection::on_get_yaml_request() { - auto *comp = store_yaml::global_store_yaml; - if (comp == nullptr || comp->get_size() == 0) { - // No component yet (request before setup()) or empty blob — send a single - // done=true response so the client always gets a terminal frame. - GetYamlResponse resp; - resp.done = true; - this->send_message(resp); - return; - } + // 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. this->store_yaml_pos_ = 0; this->try_send_store_yaml_(); } -// Caller guarantees: store_yaml_pos_ != SIZE_MAX (a stream is in progress), which -// implies on_get_yaml_request() already saw the never-cleared global non-null. +// Caller guarantees: store_yaml_pos_ != SIZE_MAX (a request is in flight). void APIConnection::try_send_store_yaml_() { auto *comp = store_yaml::global_store_yaml; - const size_t total = comp->get_size(); + // comp is only null if the request arrived before the component's setup(); + // treat that like an empty blob and send just the terminal frame. + const size_t total = comp == nullptr ? 0 : comp->get_size(); // 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 (this->store_yaml_pos_ < total) { + while (true) { if (!this->helper_->can_write_without_blocking()) return; @@ -1209,13 +1204,19 @@ void APIConnection::try_send_store_yaml_() { const size_t to_send = std::min(remaining, STORE_YAML_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(store_yaml_chunk_buf, comp->get_data() + this->store_yaml_pos_, to_send); + resp.set_data(store_yaml_chunk_buf, to_send); #else - resp.set_data(comp->get_data() + this->store_yaml_pos_, to_send); + resp.set_data(comp->get_data() + this->store_yaml_pos_, to_send); #endif - if (this->store_yaml_pos_ == 0) { + } else { + // Terminal frame for an empty blob: a valid empty pointer keeps the + // forced `data` field's memcpy well-defined. + resp.set_data(reinterpret_cast(""), 0); + } + if (this->store_yaml_pos_ == 0 && total != 0) { resp.total_size = static_cast(total); resp.encoding = StringRef(store_yaml::ENCODING); } @@ -1225,9 +1226,11 @@ void APIConnection::try_send_store_yaml_() { return; // retry on next loop, pos unchanged this->store_yaml_pos_ += to_send; + if (resp.done) + break; } - // Reached end successfully — final response (with done=true) already sent above. + // Final response (with done=true) sent successfully. this->store_yaml_pos_ = std::numeric_limits::max(); } #endif diff --git a/esphome/components/api/api_pb2.cpp b/esphome/components/api/api_pb2.cpp index d1b81d6e3a..dce2909587 100644 --- a/esphome/components/api/api_pb2.cpp +++ b/esphome/components/api/api_pb2.cpp @@ -4167,7 +4167,8 @@ uint8_t *GetYamlResponse::encode(ProtoWriteBuffer &buffer PROTO_ENCODE_DEBUG_PAR ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 10); ProtoEncode::encode_varint_raw(pos PROTO_ENCODE_DEBUG_ARG, this->data_len_); ProtoEncode::encode_raw(pos PROTO_ENCODE_DEBUG_ARG, this->data_ptr_, this->data_len_); - ProtoEncode::encode_bool(pos PROTO_ENCODE_DEBUG_ARG, 2, this->done); + ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, 16); + ProtoEncode::write_raw_byte(pos PROTO_ENCODE_DEBUG_ARG, this->done ? 0x01 : 0x00); ProtoEncode::encode_uint32(pos PROTO_ENCODE_DEBUG_ARG, 3, this->total_size); ProtoEncode::encode_string(pos PROTO_ENCODE_DEBUG_ARG, 4, this->encoding); return pos; @@ -4175,7 +4176,7 @@ uint8_t *GetYamlResponse::encode(ProtoWriteBuffer &buffer PROTO_ENCODE_DEBUG_PAR uint32_t GetYamlResponse::calculate_size() const { uint32_t size = 0; size += ProtoSize::calc_length_force(1, this->data_len_); - size += ProtoSize::calc_bool(1, this->done); + size += ProtoSize::calc_bool_force(1); size += ProtoSize::calc_uint32(1, this->total_size); size += !this->encoding.empty() ? 2 + this->encoding.size() : 0; return size; diff --git a/esphome/components/store_yaml/__init__.py b/esphome/components/store_yaml/__init__.py index f90b8807ef..b8a39ae1b7 100644 --- a/esphome/components/store_yaml/__init__.py +++ b/esphome/components/store_yaml/__init__.py @@ -1,5 +1,6 @@ from __future__ import annotations +from collections.abc import Generator from dataclasses import dataclass import logging import os @@ -150,7 +151,9 @@ def _read_files_verbatim(entries: list[tuple[str, Path]]) -> list[tuple[str, byt return files -def _iter_sensitive_values(node: object, path: tuple[str, ...] = ()): +def _iter_sensitive_values( + node: object, path: tuple[str, ...] = () +) -> Generator[tuple[tuple[str, ...], str]]: """Yield (config_path, value) for every cv.sensitive value in a config tree.""" if isinstance(node, yaml_util.SensitiveStr): yield path, str(node) @@ -201,7 +204,7 @@ def _build_secrets_skeleton(keys: set[str]) -> bytes: def _generate_redacted_files( - entries: list[tuple[str, Path]], secret_rels: set[str] + entries: list[tuple[str, Path]], secret_rels: set[str], unresolved: list[str] ) -> list[tuple[str, bytes]]: """Re-generate each captured file from its parse tree with cv.sensitive values emitted as `!secret ` references, and replace secrets files @@ -228,17 +231,34 @@ def _generate_redacted_files( for text in texts.values(): skeleton_keys |= yaml_util.find_secret_references(text) - for value, info in sensitive.items(): - # After the context manager exits, only values loaded through a real - # `!secret` are still registered — those legitimately never appear - # inline, so only warn for inline values that were never swapped. - if yaml_util.is_secret(value) is None and info.secret_name not in skeleton_keys: - _LOGGER.warning( - "store_yaml: could not locate the sensitive value of '%s' in the " - "source YAML (built via substitutions?); it may still be " - "embedded verbatim", - info.config_path, - ) + # After the context manager exits, only values loaded through a real + # `!secret` are still registered — those legitimately never appear + # inline. An inline value that was never swapped would ship verbatim in + # the blob, silently breaking the redaction promise — fail the build. + leaked = [ + info.config_path + for value, info in sensitive.items() + if yaml_util.is_secret(value) is None and info.secret_name not in skeleton_keys + ] + if leaked: + 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." + ) + + if unresolved: + # Record the gap inside the recovered config itself, not just in a + # compile-time log line: substitution-pathed includes can't be + # captured, so the user must restore those files manually. + entry_rel = entries[0][0] + texts[entry_rel] = ( + "# 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) + + texts[entry_rel] + ) skeleton = _build_secrets_skeleton(skeleton_keys) result = [ @@ -316,7 +336,7 @@ async def to_code(config: ConfigType) -> None: if config[CONF_INCLUDE_SECRETS]: files = _read_files_verbatim(entries) else: - files = _generate_redacted_files(entries, secret_rels) + files = _generate_redacted_files(entries, secret_rels, discovered.unresolved) envelope = _pack_envelope(files) compressed = zstd.compress(envelope, level=ZSTD_LEVEL) diff --git a/tests/unit_tests/components/test_store_yaml.py b/tests/unit_tests/components/test_store_yaml.py index 2e911b2d22..906ed32f07 100644 --- a/tests/unit_tests/components/test_store_yaml.py +++ b/tests/unit_tests/components/test_store_yaml.py @@ -48,8 +48,8 @@ def _sources( def _gather_redacted(discovered: DiscoveredYamlFiles) -> dict[str, bytes]: - files, secret_rels = _gather_files(discovered) - return dict(_generate_redacted_files(files, secret_rels)) + entries, secret_rels = _gather_files(discovered) + return dict(_generate_redacted_files(entries, secret_rels, discovered.unresolved)) # --------------------------------------------------------------------------- @@ -250,34 +250,37 @@ def test_redacted_reuses_existing_secret_name_for_duplicated_value( assert files["wifi.yaml"] == b"password: !secret 'api_key'\n" -def test_redacted_warns_when_value_not_locatable( - project: Path, caplog: pytest.LogCaptureFixture -) -> None: +def test_redacted_raises_when_value_not_locatable(project: Path) -> None: """A sensitive value that never appears as a whole scalar (e.g. composed - via substitutions) produces a warning naming the config path, not the value.""" + via substitutions) would ship verbatim — fail the build, naming the config + path but never the value.""" CORE.config = {"wifi": [{"password": SensitiveStr("not_in_any_file")}]} discovered = _sources(project, "wifi.yaml") - with caplog.at_level("WARNING", logger="esphome.components.store_yaml"): + with pytest.raises(EsphomeError, match="wifi.password") as err: _gather_redacted(discovered) - assert any( - "wifi.password" in r.message and "could not locate" in r.message - for r in caplog.records - ) - assert not any("not_in_any_file" in r.message for r in caplog.records) + assert "not_in_any_file" not in str(err.value) -def test_redacted_does_not_warn_for_secret_only_values( - project: Path, caplog: pytest.LogCaptureFixture -) -> None: +def test_redacted_accepts_secret_only_values(project: Path) -> None: """A value that only exists via `!secret` legitimately never appears inline.""" CORE.config = {"api": {"encryption": {"key": SensitiveStr("SUPER_SECRET")}}} yaml_util._SECRET_VALUES["SUPER_SECRET"] = "api_key" discovered = _sources( project, "entry.yaml", "secrets.yaml", secrets=("secrets.yaml",) ) - with caplog.at_level("WARNING", logger="esphome.components.store_yaml"): - _gather_redacted(discovered) - assert not any("could not locate" in r.message for r in caplog.records) + files = _gather_redacted(discovered) + assert 'api_key: ""' in files["secrets.yaml"].decode() + + +def test_redacted_records_unresolved_includes_in_entry_file(project: Path) -> None: + """Substitution-pathed includes that can't be captured are noted inside the + recovered entry file, not just in a compile-time log line.""" + discovered = _sources(project, "entry.yaml", "secrets.yaml") + discovered.unresolved.append("${board}.yaml") + files = _gather_redacted(discovered) + text = files["entry.yaml"].decode() + assert text.startswith("# store_yaml: the following !include paths") + assert "# ${board}.yaml" in text def test_redacted_skips_empty_sensitive_values(project: Path) -> None: