Skip to content

Commit 8fc1332

Browse files
fix(security): close auto_config()'s missed log-redaction cache invalidation (#5053)
Holistic pass over #5053 looking for the next Copilot-review gap before another round-trip: auto_config() (userinterface.py) is a third, independent place that writes into self.args - alongside set_arg() and web.py's apps.yaml batch editor, both already fixed - and had no invalidation at all. It resolves `re:` patterns against live HA entity ids on every startup/reconfigure pass, so a resolved or newly-unmatched-and-removed value could keep leaking under (or being missed by) a stale redaction pattern until Predbat next restarts - the same leak class this PR exists to close, just at a call site the first four review rounds didn't reach. Invalidates once at the end of the pass, only when something changed, rather than per key, since auto_config() iterates every configured arg on each call. Also documents, at the top of hass.py, why the module's "outside AppDaemon" naming does not mean the write-time redaction only partially covers the real addon population: genuine AppDaemon-hosted installs are retired, there is no appdaemon dependency in this repo, and predbat.PredBat unconditionally inherits from this file's Hass class in every currently supported install path (Predbat app/addon and Docker) - so there is no separate, unprotected logging pipeline left for the Samba-share scenario this PR targets. Added a regression test mirroring test_set_arg_invalidates_log_secret_cache: confirmed it fails with exactly the leak described (the resolved value reaches the log unredacted) when the fix is reverted, and passes with it restored. ./run_all --quick and ./run_pre_commit both green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 2dd5a06 commit 8fc1332

3 files changed

Lines changed: 82 additions & 0 deletions

File tree

‎apps/predbat/hass.py‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,18 @@
44
execution, including YAML configuration loading, secret management, log
55
rotation, scheduled callback execution, and file change detection for
66
development hot-reload.
7+
8+
Despite the "outside AppDaemon" framing (legacy naming, kept for history), this
9+
IS the class predbat.PredBat actually inherits from in every currently
10+
supported install path - the Predbat app/addon and Docker both run this
11+
standalone-style loader, not a real appdaemon package. The genuinely
12+
AppDaemon-hosted install method has been retired (docs/install.md); there is
13+
no appdaemon dependency anywhere in this repo, and no conditional import
14+
branches to a different hass module. So Hass.log() below - and the write-time
15+
secret redaction in it (GH#4770) - is not a partial mitigation that misses an
16+
AppDaemon-hosted population still running elsewhere: there is no such
17+
population left to miss. Flagging this explicitly because the class/module
18+
docstrings alone would lead a reviewer to (reasonably) suspect the opposite.
719
"""
820

921
import io

‎apps/predbat/tests/test_secrets.py‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -615,6 +615,60 @@ def test_set_arg_invalidates_log_secret_cache(my_predbat):
615615
return failed
616616

617617

618+
def test_auto_config_invalidates_log_secret_cache(my_predbat):
619+
"""auto_config() must invalidate log()'s cached redaction pattern too (holistic #5053 review).
620+
621+
set_arg() and the web.py apps.yaml batch editor both invalidate the cache on every args
622+
mutation (Copilot review on #5053) - but auto_config() (userinterface.py) is a third,
623+
independent place that writes into self.args: it resolves `re:` patterns against live HA
624+
entity ids on every startup/reconfigure pass (predbat.py's auto_config()/auto_config(final=True)
625+
calls) and had no invalidation at all. A `re:` argument that resolves to (or stops matching,
626+
and is removed from self.args) a credential-shaped value would otherwise keep leaking under -
627+
or being missed by - a stale pattern built before auto_config() ran, until Predbat next
628+
restarts. Covers both the "resolved to a new value" and "removed because nothing matched" arms.
629+
"""
630+
if my_predbat is None:
631+
return False
632+
print("**** Testing auto_config() invalidates the cached log redaction pattern ****")
633+
failed = False
634+
635+
import io
636+
637+
saved_args = my_predbat.args.copy()
638+
saved_logfile = my_predbat.logfile
639+
saved_cache = my_predbat._log_secret_pattern_cache
640+
saved_get_state_wrapper = my_predbat.get_state_wrapper
641+
saved_unmatched_args = getattr(my_predbat, "unmatched_args", {})
642+
try:
643+
my_predbat.args.pop("test_marker_secret_key_xyz", None)
644+
my_predbat.args["test_marker_secret_key_xyz"] = "re:sensor\\.auto_config_secret_marker"
645+
my_predbat.get_state_wrapper = lambda *a, **kw: {"sensor.auto_config_secret_marker": "on"}
646+
my_predbat._log_secret_pattern_cache = my_predbat._LOG_SECRET_PATTERN_UNSET
647+
my_predbat.logfile = io.StringIO()
648+
649+
# Build the cache from the pre-auto_config() args (the "re:..." string itself, not yet
650+
# resolved), the same way a real log() call between startup and auto_config() would.
651+
my_predbat.log("Info: nothing secret published yet")
652+
my_predbat.auto_config()
653+
my_predbat.log("Info: now using sensor.auto_config_secret_marker")
654+
655+
content = my_predbat.logfile.getvalue()
656+
if "sensor.auto_config_secret_marker" in content and "<test_marker_secret_key_xyz>" not in content:
657+
print("ERROR: a value resolved by auto_config() after the cache was built still leaked into the log unredacted: {}".format(content))
658+
failed = True
659+
finally:
660+
my_predbat.args.clear()
661+
my_predbat.args.update(saved_args)
662+
my_predbat.unmatched_args = saved_unmatched_args
663+
my_predbat.get_state_wrapper = saved_get_state_wrapper
664+
my_predbat.logfile = saved_logfile
665+
my_predbat._log_secret_pattern_cache = saved_cache
666+
667+
if not failed:
668+
print("**** test_auto_config_invalidates_log_secret_cache PASSED ****")
669+
return failed
670+
671+
618672
def test_log_secret_pattern_build_is_not_racy(my_predbat):
619673
"""_log_secret_pattern() must not let a build started before an invalidation overwrite that
620674
invalidation once it finishes (#5053 review).
@@ -706,5 +760,6 @@ def run_secrets_tests(my_predbat=None):
706760
failed |= test_log_redacts_at_write_time()
707761
failed |= test_redact_strings_masked_in_debug_dump()
708762
failed |= test_set_arg_invalidates_log_secret_cache(my_predbat)
763+
failed |= test_auto_config_invalidates_log_secret_cache(my_predbat)
709764
failed |= test_log_secret_pattern_build_is_not_racy(my_predbat)
710765
return failed

‎apps/predbat/userinterface.py‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1214,6 +1214,7 @@ def auto_config(self, final=False):
12141214
self.unmatched_args = {}
12151215

12161216
# Find each arg re to match
1217+
changed = False
12171218
for arg in self.args:
12181219
arg_value = self.args[arg]
12191220
matched, arg_value = self.resolve_arg_re(arg, arg_value, state_keys)
@@ -1223,11 +1224,25 @@ def auto_config(self, final=False):
12231224
disabled.append(arg)
12241225
else:
12251226
self.args[arg] = arg_value
1227+
changed = True
12261228

12271229
# Remove unmatched keys
12281230
for key in disabled:
12291231
self.unmatched_args[key] = self.args[key]
12301232
del self.args[key]
1233+
changed = True
1234+
1235+
# A `re:` pattern can resolve to a live HA entity's state/attribute - a credential-shaped
1236+
# value from a third-party integration is exactly what redact_strings/redact_strings_labelled
1237+
# exist to catch (GH#4770) - and a disabled key's removal changes what collect_log_secret_values()
1238+
# sees from args too. Unlike set_arg() (this class's only other args mutator, which invalidates
1239+
# unconditionally on every call), auto_config() runs over every configured arg on each call, so
1240+
# invalidating once at the end - only when something actually changed - avoids rebuilding the
1241+
# pattern key-by-key while still closing the gap: a value resolved or removed here must not keep
1242+
# leaking under the stale pattern (or keep being redacted after a redact_strings entry is removed)
1243+
# until Predbat next restarts.
1244+
if changed:
1245+
self._invalidate_log_secret_pattern()
12311246

12321247
def split_command_index(self, command):
12331248
"""

0 commit comments

Comments
 (0)