diff --git a/apps/predbat/chat_tools.py b/apps/predbat/chat_tools.py index e9d04da55..c37c6c1a8 100644 --- a/apps/predbat/chat_tools.py +++ b/apps/predbat/chat_tools.py @@ -772,7 +772,7 @@ def set_apps_config(base, key, value, apps_yaml_path=APPS_YAML_PATH, backup_path base.args[key] = value # The written key can itself be a credential (or the redact_strings/redact_strings_labelled - # denylists), so log()'s cached redaction pattern (hass.py) must be rebuilt on next use or a + # denylists), so log()'s cached redaction pattern (log_secrets.py) must be rebuilt on next use or a # newly saved value keeps leaking into the log under the stale pattern until restart (#5053 # review). base._invalidate_log_secret_pattern() diff --git a/apps/predbat/hass.py b/apps/predbat/hass.py index 581090a29..fbe4f5163 100644 --- a/apps/predbat/hass.py +++ b/apps/predbat/hass.py @@ -24,7 +24,8 @@ import os import subprocess -from utils import collect_log_secret_values, compile_log_secret_pattern, redact_log_line +from log_secrets import LogRedaction +from utils import redact_log_line def write_git_version_marker(): @@ -183,82 +184,17 @@ async def main(): sys.exit(0) -class Hass: +class Hass(LogRedaction): """Standalone mode wrapper emulating the AppDaemon interface. Enables PredBat to run outside Home Assistant/AppDaemon with YAML config loading, secret management, log rotation, scheduled callbacks, and file change detection for development hot-reload. - """ - - # Sentinel distinct from None: compile_log_secret_pattern() legitimately returns None when - # there are no secrets configured to redact, so None alone in the cache slot can't tell - # "not built yet" from "built, and there is nothing to redact" - the latter would otherwise - # rebuild (recompute the value set, recompile) on every single log() call instead of caching. - _LOG_SECRET_PATTERN_UNSET = object() - - def _invalidate_log_secret_pattern(self): - """ - Mark the cached redaction pattern stale so the next log() call rebuilds it from the - current args/secrets (GH#4770). Every call site that mutates self.args or self.secrets - after startup must call this - see _log_secret_pattern()'s docstring for why a missed - site is a real leak, not just a staleness bug. - """ - with self._log_secret_pattern_lock: - self._log_secret_pattern_cache = self._LOG_SECRET_PATTERN_UNSET - def _log_secret_fingerprint(self): - """ - A cheap value that changes whenever the set of credentials in args/secrets could have. - - Deliberately not a hash of every value: this runs under the lock on the way to each - rebuild decision, so it stays O(number of top-level keys). Identity of the args and - secrets mappings plus their sizes catches the shapes a config mutation takes - a key - added or removed (size), and the whole mapping being replaced or rebound (identity), as - web.py's batch editor does via clear()/update() and web_chat.py does by assigning a new - block. - - An in-place edit of an existing key that keeps the size the same is NOT caught here, so - the explicit _invalidate_log_secret_pattern() call sites remain load-bearing. This is a - safety net under them, not a replacement: with it, a missed or mis-ordered call site - degrades to "redacted from the next line" instead of "leaks until the next restart", - which is the failure mode successive reviews of this PR kept finding one call site at a - time. GH#5063 tracks removing the contract itself by routing every args mutation through - one setter (#5053 review). - """ - args = getattr(self, "args", None) - secrets = getattr(self, "secrets", None) - return (id(args), len(args) if isinstance(args, dict) else -1, id(secrets), len(secrets) if isinstance(secrets, dict) else -1) - - def _log_secret_pattern(self): - """ - Return the cached compiled redaction pattern log() must apply, rebuilding it the first - time it is needed and whenever load_secrets()/apps.yaml load invalidate it (GH#4770). - - Cached rather than recomputed on every log() call: log() runs on every log line, while - args/secrets only change on startup and on a config reload, so rebuilding the value set - and recompiling the pattern that rarely - rather than on every call - keeps the - redaction check to a single compiled-regex scan per line on the hot path. - - Guarded by a lock, not just the sentinel check: log() runs from component threads as well - as the main thread (create_task()), so two threads can both observe the sentinel and race - to rebuild. Without the lock, a thread that started building from stale args right before - another thread invalidates the cache (a credential just added via set_arg()) can finish - second and overwrite the fresh invalidation with its stale, already-out-of-date pattern - - silently keeping the just-added credential unredacted until something invalidates the - cache again. The lock makes "read sentinel, build, store" one atomic step so a build that - started before an invalidation can never win a race against it. - """ - with self._log_secret_pattern_lock: - fingerprint = self._log_secret_fingerprint() - if self._log_secret_pattern_cache is self._LOG_SECRET_PATTERN_UNSET or fingerprint != self._log_secret_pattern_fingerprint: - args = getattr(self, "args", None) - redact_strings = args.get("redact_strings") if args else None - redact_strings_labelled = args.get("redact_strings_labelled") if args else None - values = collect_log_secret_values(args, getattr(self, "secrets", None), redact_strings, redact_strings_labelled) - self._log_secret_pattern_cache = compile_log_secret_pattern(values) - self._log_secret_pattern_fingerprint = fingerprint - return self._log_secret_pattern_cache + log()'s credential-redaction cache lives in LogRedaction (log_secrets.py) rather + than here, so it carries its own state and needs nothing from this __init__ + (GH#5169). + """ def log(self, msg, quiet=True): """ @@ -338,9 +274,6 @@ def __init__(self): self.threads = [] self.fatal_error = False self.hass_api_version = 2 - self._log_secret_pattern_lock = threading.Lock() - self._log_secret_pattern_cache = self._LOG_SECRET_PATTERN_UNSET - self._log_secret_pattern_fingerprint = None self.logfile = open("predbat.log", "a") diff --git a/apps/predbat/log_secrets.py b/apps/predbat/log_secrets.py new file mode 100644 index 000000000..0e2994e84 --- /dev/null +++ b/apps/predbat/log_secrets.py @@ -0,0 +1,154 @@ +# ----------------------------------------------------------------------------- +# Predbat Home Battery System +# Copyright Trefor Southwell 2026 - All Rights Reserved +# This application maybe used for personal use only and not for commercial use +# ----------------------------------------------------------------------------- +# fmt off +# pylint: disable=consider-using-f-string +# pylint: disable=line-too-long +# pylint: disable=attribute-defined-outside-init + + +"""Cached credential-redaction pattern for log(), as a self-contained mixin. + +Holds the compiled pattern Hass.log() applies to every line it writes (GH#4770), kept +here rather than on Hass itself so that whatever class provides log() only has to +inherit LogRedaction to get working redaction - with no __init__ of its own to +remember (GH#5169). +""" + +import threading + +from utils import collect_log_secret_values, compile_log_secret_pattern + + +class LogRedaction: + """Mixin providing log()'s cached credential-redaction pattern (GH#4770). + + Inheritable on its own terms: all of its state is established on the class rather + than in an __init__, so a class that inherits this mixin has working redaction + whatever its own constructor does - including not calling this one's, since there + isn't one. That matters because the members below are called from files that do not + define the class holding them (userinterface.py, web.py, chat_tools.py all call + _invalidate_log_secret_pattern() on the engine object), so "has the methods but not + the state" was a reachable state as long as exactly one __init__ established it, and + an engine composed from a different hass.py crash-looped on precisely that (GH#5169). + """ + + # Sentinel distinct from None: compile_log_secret_pattern() legitimately returns None when + # there are no secrets configured to redact, so None alone in the cache slot can't tell + # "not built yet" from "built, and there is nothing to redact" - the latter would otherwise + # rebuild (recompute the value set, recompile) on every single log() call instead of caching. + _LOG_SECRET_PATTERN_UNSET = object() + + # Class attributes, not __init__ assignments: an instance that has never built a pattern + # reads these, and the first build/invalidation shadows them with instance attributes of its + # own, so the cache is per-instance from the moment it holds anything instance-specific + # while still needing no constructor to exist (GH#5169). + _log_secret_pattern_cache = _LOG_SECRET_PATTERN_UNSET + _log_secret_pattern_fingerprint = None + + # The lock, unlike the cache, cannot be created on first use: _log_secret_pattern() takes it + # on entry precisely to serialise concurrent first uses, and two threads lazily creating one + # each would each proceed inside their own - the exact interleaving it exists to prevent. A + # class attribute is therefore shared by every instance, which is sound because all it guards + # is the atomicity of one instance's "read sentinel, build, store"; making that atomic + # process-wide is strictly stronger than making it per-instance. + # + # It is acquired on every _log_secret_pattern() call, so on every log line, and held across + # the fingerprint check - a handful of identity and len() reads - on the warm path; only a + # rebuild holds it for longer, and rebuilds happen on a config change. Sharing it does mean a + # rebuild on one instance briefly blocks log() on every other instance of the class, which + # the single-engine production case never notices but a test process holding several engines + # could (#5171 review). Nothing reached under the lock logs - collect_log_secret_values() and + # compile_log_secret_pattern() are pure helpers in utils.py - so there is no re-entrant path + # back into log() to deadlock a non-reentrant Lock, which is what makes sharing it safe + # rather than merely cheap. + _log_secret_pattern_lock = threading.Lock() + + def _invalidate_log_secret_pattern(self): + """ + Mark the cached redaction pattern stale so the next log() call rebuilds it from the + current args/secrets (GH#4770). Every call site that mutates self.args or self.secrets + after startup must call this - see _log_secret_pattern()'s docstring for why a missed + site is a real leak, not just a staleness bug. + """ + with self._log_secret_pattern_lock: + self._log_secret_pattern_cache = self._LOG_SECRET_PATTERN_UNSET + + def _log_secret_fingerprint(self): + """ + A cheap value that changes whenever the set of credentials in args/secrets could have. + + Deliberately not a hash of every value: this runs under the lock on the way to each + rebuild decision, so it stays O(number of top-level keys). Identity of the args and + secrets mappings plus their sizes catches the shapes a config mutation takes - a key + added or removed (size), and the whole mapping being replaced or rebound (identity), as + web.py's batch editor does via clear()/update() and web_chat.py does by assigning a new + block. + + The mappings themselves are held here, not their id(): a stored id() is only an integer, + so once the mapping it was taken from is freed CPython is free to hand that same address + to the next allocation, and a rebind to a same-length dict landing there would produce a + fingerprint equal to the stored one - skipping the rebuild and leaving a just-configured + credential unredacted until the next explicit invalidation (#5171 review). Holding the + objects means an address recorded in a live fingerprint cannot be recycled while it is + still recorded, at the cost of keeping a superseded args/secrets mapping reachable until + the next rebuild. They are compared by _log_secret_fingerprint_matches() rather than ==, + because == on two dicts is an O(number of values) comparison that would also report two + distinct-but-equal mappings as unchanged - identity is the question being asked. + + An in-place edit of an existing key that keeps the size the same is NOT caught here, so + the explicit _invalidate_log_secret_pattern() call sites remain load-bearing. This is a + safety net under them, not a replacement: with it, a missed or mis-ordered call site + degrades to "redacted from the next line" instead of "leaks until the next restart", + which is the failure mode successive reviews of #5053 kept finding one call site at a + time. GH#5063 tracks removing the contract itself by routing every args mutation through + one setter (#5053 review). + """ + args = getattr(self, "args", None) + secrets = getattr(self, "secrets", None) + return (args, len(args) if isinstance(args, dict) else -1, secrets, len(secrets) if isinstance(secrets, dict) else -1) + + @staticmethod + def _log_secret_fingerprint_matches(left, right): + """ + Whether two fingerprints from _log_secret_fingerprint() describe the same args/secrets. + + The mapping halves are compared with `is` and only their sizes with ==, so a rebind to an + equal-but-distinct mapping counts as a change (a rebuild) rather than a match. `right` is + None before the first build, which never matches a real fingerprint. + """ + if left is None or right is None: + return left is right + return left[0] is right[0] and left[1] == right[1] and left[2] is right[2] and left[3] == right[3] + + def _log_secret_pattern(self): + """ + Return the cached compiled redaction pattern log() must apply, rebuilding it the first + time it is needed and whenever load_secrets()/apps.yaml load invalidate it (GH#4770). + + Cached rather than recomputed on every log() call: log() runs on every log line, while + args/secrets only change on startup and on a config reload, so rebuilding the value set + and recompiling the pattern that rarely - rather than on every call - keeps the + redaction check to a single compiled-regex scan per line on the hot path. + + Guarded by a lock, not just the sentinel check: log() runs from component threads as well + as the main thread (create_task()), so two threads can both observe the sentinel and race + to rebuild. Without the lock, a thread that started building from stale args right before + another thread invalidates the cache (a credential just added via set_arg()) can finish + second and overwrite the fresh invalidation with its stale, already-out-of-date pattern - + silently keeping the just-added credential unredacted until something invalidates the + cache again. The lock makes "read sentinel, build, store" one atomic step so a build that + started before an invalidation can never win a race against it. + """ + with self._log_secret_pattern_lock: + fingerprint = self._log_secret_fingerprint() + if self._log_secret_pattern_cache is self._LOG_SECRET_PATTERN_UNSET or not self._log_secret_fingerprint_matches(fingerprint, self._log_secret_pattern_fingerprint): + args = getattr(self, "args", None) + redact_strings = args.get("redact_strings") if args else None + redact_strings_labelled = args.get("redact_strings_labelled") if args else None + values = collect_log_secret_values(args, getattr(self, "secrets", None), redact_strings, redact_strings_labelled) + self._log_secret_pattern_cache = compile_log_secret_pattern(values) + self._log_secret_pattern_fingerprint = fingerprint + return self._log_secret_pattern_cache diff --git a/apps/predbat/tests/test_secrets.py b/apps/predbat/tests/test_secrets.py index 3e60849a8..5b4b02c0e 100644 --- a/apps/predbat/tests/test_secrets.py +++ b/apps/predbat/tests/test_secrets.py @@ -799,10 +799,13 @@ def test_log_secret_pattern_build_is_not_racy(my_predbat): print("**** Testing _log_secret_pattern() build is not racy against a concurrent invalidation ****") failed = False - import hass as hass_module + # Patched where _log_secret_pattern() looks the name up, which is log_secrets.py's module + # namespace since the cache moved out of Hass (GH#5169) - patching hass.py's copy would no + # longer pause the build at all, and the test would silently stop exercising the race. + import log_secrets as log_secrets_module saved_cache = my_predbat._log_secret_pattern_cache - saved_collect = hass_module.collect_log_secret_values + saved_collect = log_secrets_module.collect_log_secret_values build_started = _threading.Event() release_build = _threading.Event() @@ -813,7 +816,7 @@ def paused_collect(*args, **kwargs): try: my_predbat._log_secret_pattern_cache = my_predbat._LOG_SECRET_PATTERN_UNSET - hass_module.collect_log_secret_values = paused_collect + log_secrets_module.collect_log_secret_values = paused_collect builder = _threading.Thread(target=my_predbat._log_secret_pattern) builder.start() @@ -849,7 +852,7 @@ def paused_collect(*args, **kwargs): print("ERROR: a build that started before an invalidation was allowed to overwrite that invalidation's result") failed = True finally: - hass_module.collect_log_secret_values = saved_collect + log_secrets_module.collect_log_secret_values = saved_collect my_predbat._log_secret_pattern_cache = saved_cache if not failed: @@ -857,6 +860,224 @@ def paused_collect(*args, **kwargs): return failed +def test_log_redaction_mixin_needs_no_init(): + """The redaction cache must work on any object that inherits the mixin, whatever its own + __init__ does (GH#5169). + + The three members that drive it are called from files that do not define the class holding + them - userinterface.py's set_arg()/auto_config(), web.py's batch editor and chat_tools.py + all call _invalidate_log_secret_pattern() on the engine object - so while exactly one + __init__ established the state they need, "has the methods but not the state" was a + reachable state. An engine composed from a replacement hass.py crash-looped on precisely + that, during init, because set_arg() runs there. + + Two things are asserted here, matching the two ways the mixin is meant to be usable: + + - a Hass subclass that never calls Hass.__init__ still redacts, rather than raising + AttributeError on the first invalidation or log line; + - LogRedaction is usable on its own, with no Hass anywhere, and each instance's pattern + covers its own args only - the class attributes that replace the __init__ assignments are + defaults to read, not shared state to write into. + """ + print("**** Testing the log redaction mixin works without Hass.__init__ ****") + failed = False + + import io + from utils import redact_log_line + + class EngineWithItsOwnInit(Hass): + """A Hass subclass that establishes its own state instead of calling Hass.__init__.""" + + def __init__(self): + """Set up just enough state to log, deliberately without calling Hass.__init__.""" + self.args = {"octopus_api_key": "OVERLAY-SECRET-VALUE-5169"} + self.secrets = {} + self.logfile = io.StringIO() + + engine = EngineWithItsOwnInit() + try: + # The exact call set_arg() makes after mutating args - where the reported crash surfaced. + engine._invalidate_log_secret_pattern() + engine.log("Info: connecting with OVERLAY-SECRET-VALUE-5169", quiet=True) + except AttributeError as error: + print("ERROR: redaction needed state that only Hass.__init__ establishes: {}".format(error)) + failed = True + except Exception as error: + # Not just AttributeError: log() has other ways to fail on an engine that skipped + # Hass.__init__ (a TypeError from an unexpected mixin-state shape, predbat_log_count()), + # and letting one escape would abort the whole suite run instead of reporting a failed + # test like every other case here (#5171 review). + print("ERROR: logging on an engine that skipped Hass.__init__ raised {}: {}".format(type(error).__name__, error)) + failed = True + else: + content = engine.logfile.getvalue() + if "OVERLAY-SECRET-VALUE-5169" in content: + print("ERROR: a secret leaked into the log of an engine that skipped Hass.__init__: {}".format(content)) + failed = True + if "" not in content: + print("ERROR: the redaction lost its credential label: {}".format(content)) + failed = True + + try: + from log_secrets import LogRedaction + except ImportError as error: + print("ERROR: the redaction mixin is not importable on its own: {}".format(error)) + return True + + class BareRedactor(LogRedaction): + """An object that inherits the redaction mixin and nothing else - no Hass involved.""" + + def __init__(self, args): + """Hold the args the redaction pattern is built from.""" + self.args = args + + first = BareRedactor({"api_key": "FIRST-INSTANCE-SECRET-5169"}) + second = BareRedactor({"api_key": "SECOND-INSTANCE-SECRET-5169"}) + + first_line = redact_log_line("Info: using FIRST-INSTANCE-SECRET-5169", first._log_secret_pattern()) + if "FIRST-INSTANCE-SECRET-5169" in first_line or "" not in first_line: + print("ERROR: the mixin did not redact on its own, without a Hass: {}".format(first_line)) + failed = True + + # Built after the first instance's: each object's pattern must cover its own args and only + # its own, so the class attributes that stand in for the removed __init__ assignments stay + # defaults every instance reads rather than one slot they all write into. + second_line = redact_log_line("Info: using SECOND-INSTANCE-SECRET-5169 with FIRST-INSTANCE-SECRET-5169", second._log_secret_pattern()) + if "SECOND-INSTANCE-SECRET-5169" in second_line: + print("ERROR: the second instance did not build its own pattern: {}".format(second_line)) + failed = True + if "FIRST-INSTANCE-SECRET-5169" not in second_line: + print("ERROR: the second instance is redacting the first instance's secrets - the cache is shared, not per-instance: {}".format(second_line)) + failed = True + + if not failed: + print("**** test_log_redaction_mixin_needs_no_init PASSED ****") + return failed + + +def test_log_secret_pattern_cache_is_reused(): + """A second _log_secret_pattern() call must return the cached pattern without rebuilding it. + + The rest of this suite asserts the cold path (a build happens), the invalidation call sites and + the fingerprint safety net, but nothing counted rebuilds - so a regression that recomputed the + value set and recompiled the regex on every log line, which is the one thing the cache exists + to avoid on log()'s hot path, would have passed the whole suite silently (#5171 review). + """ + print("**** Testing the log redaction pattern is cached across calls ****") + failed = False + + # Patched in log_secrets.py's namespace, where _log_secret_pattern() looks the name up, for the + # same reason as test_log_secret_pattern_build_is_not_racy(). + import log_secrets as log_secrets_module + from log_secrets import LogRedaction + + class CountingRedactor(LogRedaction): + """A mixin-only object, so a shared engine's own logging cannot disturb the build count.""" + + def __init__(self, args): + """Hold the args the redaction pattern is built from.""" + self.args = args + + saved_collect = log_secrets_module.collect_log_secret_values + builds = [] + + def counting_collect(*args, **kwargs): + """Count each rebuild on the way through to the real collector.""" + builds.append(1) + return saved_collect(*args, **kwargs) + + try: + log_secrets_module.collect_log_secret_values = counting_collect + redactor = CountingRedactor({"api_key": "CACHE-REUSE-SECRET-5171"}) + + first = redactor._log_secret_pattern() + if len(builds) != 1: + print("ERROR: the first call made {} builds, expected exactly 1".format(len(builds))) + failed = True + + stored_fingerprint = redactor._log_secret_pattern_fingerprint + second = redactor._log_secret_pattern() + if len(builds) != 1: + print("ERROR: a second call rebuilt the pattern instead of returning the cached one - {} builds".format(len(builds))) + failed = True + if second is not first: + print("ERROR: a second call returned a different pattern object from the one it cached") + failed = True + if redactor._log_secret_pattern_fingerprint is not stored_fingerprint: + print("ERROR: a cache hit replaced the stored fingerprint, so it did not take the cached path") + failed = True + + # A cache that can never be rebuilt would satisfy everything above just as well as a + # correct one, so confirm the counter still moves when it is supposed to. + redactor._invalidate_log_secret_pattern() + redactor._log_secret_pattern() + if len(builds) != 2: + print("ERROR: the pattern was not rebuilt after an invalidation - {} builds".format(len(builds))) + failed = True + finally: + log_secrets_module.collect_log_secret_values = saved_collect + + if not failed: + print("**** test_log_secret_pattern_cache_is_reused PASSED ****") + return failed + + +def test_log_secret_fingerprint_is_not_fooled_by_address_reuse(): + """Rebinding args to a mapping that could land on the freed one's address must still rebuild. + + The fingerprint is the safety net under the explicit invalidation call sites, so a false + "unchanged" verdict there is a leak rather than mere staleness: the rebuild is skipped and a + just-configured credential stays in plaintext until something else invalidates the cache. + Recording id(args) permitted exactly that - the id is only an integer, so CPython may hand the + freed mapping's address to the next allocation and a same-length replacement landing there + compares equal (#5171 review). The fingerprint holds the mappings themselves, which is what + keeps an address it recorded from being recycled while it is still recorded. + """ + print("**** Testing the redaction fingerprint is not fooled by a recycled mapping address ****") + failed = False + + import gc + from utils import redact_log_line + from log_secrets import LogRedaction + + class BareRedactor(LogRedaction): + """An object carrying args and the redaction mixin, and nothing else.""" + + def __init__(self, args): + """Hold the args the redaction pattern is built from.""" + self.args = args + + redactor = BareRedactor({"api_key": "FIRST-ADDRESS-SECRET-5171"}) + if "FIRST-ADDRESS-SECRET-5171" in redact_log_line("Info: using FIRST-ADDRESS-SECRET-5171", redactor._log_secret_pattern()): + print("ERROR: the first pattern did not redact the args it was built from") + failed = True + + # Rebind args and keep hold of the superseded mapping only through this local, so what the + # stored fingerprint retains is the question. Asserted by referrer rather than by trying to + # provoke a real address collision: whether CPython actually hands the freed address to the + # next allocation depends on the allocator's pool state, so a probe that allocates same-shaped + # dicts and looks for the old address passes with or without the fix and proves nothing. The + # property that rules the collision out is that the recorded address still belongs to a live + # object, which is exactly "the fingerprint refers to the mapping". + superseded_args = redactor.args + redactor.args = {"api_key": "SECOND-ADDRESS-SECRET-5171"} + gc.collect() + + stored_fingerprint = redactor._log_secret_pattern_fingerprint + if not any(referrer is stored_fingerprint for referrer in gc.get_referrers(superseded_args)): + print("ERROR: the stored fingerprint does not refer to the mapping it describes, so that mapping can be freed and its address handed to a same-length replacement - which would compare as unchanged and skip the rebuild") + failed = True + + second_line = redact_log_line("Info: using SECOND-ADDRESS-SECRET-5171", redactor._log_secret_pattern()) + if "SECOND-ADDRESS-SECRET-5171" in second_line or "" not in second_line: + print("ERROR: rebinding args to a same-length mapping did not rebuild the pattern: {}".format(second_line)) + failed = True + + if not failed: + print("**** test_log_secret_fingerprint_is_not_fooled_by_address_reuse PASSED ****") + return failed + + def test_resolve_arg_re_masks_secret_arg_matches(my_predbat): """ resolve_arg_re() logs the entity id it matched before auto_config()'s caller has invalidated @@ -912,6 +1133,9 @@ def run_secrets_tests(my_predbat=None): failed |= test_set_arg_invalidates_log_secret_cache(my_predbat) failed |= test_log_redaction_survives_a_missed_invalidation(my_predbat) failed |= test_auto_config_invalidates_log_secret_cache(my_predbat) + failed |= test_log_redaction_mixin_needs_no_init() + failed |= test_log_secret_pattern_cache_is_reused() + failed |= test_log_secret_fingerprint_is_not_fooled_by_address_reuse() failed |= test_log_secret_pattern_build_is_not_racy(my_predbat) failed |= test_resolve_arg_re_masks_secret_arg_matches(my_predbat) return failed diff --git a/apps/predbat/userinterface.py b/apps/predbat/userinterface.py index c4ea4fd01..5e7b28acd 100644 --- a/apps/predbat/userinterface.py +++ b/apps/predbat/userinterface.py @@ -214,7 +214,7 @@ def set_arg(self, arg, value, index=None): else: self.args[arg] = value # A credential value or the redact_strings/redact_strings_labelled denylists themselves - # can change here, so log()'s cached redaction pattern (hass.py) must be rebuilt on next + # can change here, so log()'s cached redaction pattern (log_secrets.py) must be rebuilt on next # use - otherwise a newly added/changed secret keeps leaking into the log under the stale # pattern until Predbat restarts (GH#4770 review). self._invalidate_log_secret_pattern() diff --git a/apps/predbat/utils.py b/apps/predbat/utils.py index 03b380ab4..34186a651 100644 --- a/apps/predbat/utils.py +++ b/apps/predbat/utils.py @@ -608,7 +608,7 @@ def compile_log_secret_pattern(secret_values): Compile the {value: label} map into a single alternation pattern plus a value->label lookup for redact_log_line(), or None when there is nothing to redact. - Compiled once whenever the value set changes (hass.py caches this alongside the values + Compiled once whenever the value set changes (log_secrets.py caches this alongside the values themselves) rather than per log line: log() runs on every line, and matching one compiled alternation is a single scan of the line regardless of how many secrets there are to check for, where re-scanning the line once per value (the naive str.replace() loop) costs O(line diff --git a/apps/predbat/web.py b/apps/predbat/web.py index 851b76105..2e59223dd 100644 --- a/apps/predbat/web.py +++ b/apps/predbat/web.py @@ -4171,7 +4171,7 @@ async def html_apps_post(self, request): self.args.update(live_args) # A credential value or the redact_strings/redact_strings_labelled denylists # themselves can change in this batch, so log()'s cached redaction pattern - # (hass.py, held on self.base - the PredBat instance, not this web component) + # (log_secrets.py, held on self.base - the PredBat instance, not this web component) # must be rebuilt on next use, or a newly added/changed secret keeps leaking into # the log under the stale pattern until the restart below completes (GH#4770 review). self.base._invalidate_log_secret_pattern()