fix(security): address the follow-up Copilot review on #5053 - numeric secrets, overlap redaction, a load_secrets() crash, a cache race, and a label-priority bug

Six findings from a second round of Copilot review on the #4770 write-time log
redaction PR, all confirmed and fixed.

Numeric secrets.yaml/redact_strings_labelled values silently dropped (utils.py)
-----------------------------------------------------------------------------
collect_log_secret_values() checked isinstance(value, str), so an unquoted
numeric MPAN/account ID (landlord_mpan: 1234567890123) loaded from YAML as an
int and was silently skipped - never redacted even though log() serializes
every message with str(msg). Scalar secrets.yaml and redact_strings_labelled
values are now coerced to string before matching (bools excluded, to avoid
"True"/"False" matching ordinary log text).

redact_strings_labelled schema gave no warning for a non-string value (config.py, predbat.py)
-----------------------------------------------------------------------------------------------
The dict-type validator only checked the mapping itself, never its values, so
the numeric-MPAN case above passed validation with no warning at all. Added a
scalar_value_dict schema flag, checked in validate_config()'s dict branch,
that warns (not errors - the value is still usable once coerced) when a
mapping value isn't already a string.

Two overlapping secrets at different start positions left part of the longer one exposed (utils.py)
-------------------------------------------------------------------------------------------------------
pattern.sub() finds the first alternative that matches at the earliest
position and resumes scanning after it, so "sec1" and "c123x" both present in
"sec123x" only ever matched "sec1" - "c123x" starting one character in was
never considered, leaking "23x". redact_log_line() now extends each match to
the longest secret starting anywhere inside its span before emitting the
mask, chasing chained overlaps until the span stabilises.

A malformed secrets.yaml (valid YAML, wrong shape) crashed startup entirely (hass.py)
-----------------------------------------------------------------------------------------
load_secrets() only guarded yaml.safe_load()'s exceptions, not a load that
succeeds with the wrong shape - a bare scalar/list secrets.yaml reassigns the
local `secrets` before the next line's secrets.get("logger") call throws
AttributeError. The except caught that, but the reassignment had already
happened, so load_secrets() still returned the malformed value - and the very
next log() call crashed unhandled reaching collect_log_secret_values()'s
secrets.items(). Now checked explicitly and degraded to {} with a warning.

The cached redaction pattern build was racy against a concurrent invalidation (hass.py)
---------------------------------------------------------------------------------------------
log() runs from component threads as well as the main thread, so two threads
could both observe the UNSET sentinel and race to rebuild: a thread that
started building from stale args right before another thread invalidated the
cache could finish second and overwrite the fresh invalidation with its
stale pattern, silently keeping a just-added credential unredacted. Added a
lock making "read sentinel, build, store" one atomic step, and two missed
invalidation call sites (web_chat.py's provider save, chat_tools.py's
set_apps_config) that could leave a saved credential under the stale pattern.

redact_strings raced its own more-specific label (utils.py)
-------------------------------------------------------------
redact_strings/redact_strings_labelled are secret-flagged (via
SECRET_KEY_EXPLICIT_NAMES) so mask_secret_args()'s debug-dump masking hides
them wholesale - but that same flag made the generic args-traversal pass in
_collect_secret_values() pick up redact_strings as an ordinary secret-valued
key too, labelling its values generically BEFORE the dedicated
redact_strings_labelled pass ran with its documented "specific label wins"
priority. A value present in both denylist forms kept the generic label
instead. The two true top-level keys are now skipped in that traversal (an
unrelated nested key sharing the name still collects normally).

All six verified with mutation testing against the actual fix: reverting each
one in turn reproduces exactly the failure the corresponding new/extended
test describes, and only that test fails.

./run_all --quick and ./run_pre_commit both green.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

Author: chalfontchubby

apps/predbat/chat_tools.py

@@ -771,6 +771,11 @@ def set_apps_config(base, key, value, apps_yaml_path=APPS_YAML_PATH, backup_path
         return {"success": False, "error": "Could not write apps.yaml: {}".format(error), "data": None}
 
     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
+    # newly saved value keeps leaking into the log under the stale pattern until restart (#5053
+    # review).
+    base._invalidate_log_secret_pattern()
 
     description = "Changed apps.yaml key '{}' from {!r} to {!r}. {}".format(key, previous_value, value, APPS_YAML_RESTART_WARNING)
     return {

apps/predbat/config.py

@@ -2806,5 +2806,5 @@
     # <your_label> instead of the generic <redact_strings>, the same way a built-in credential is
     # labelled by its own apps.yaml key name - e.g. "my_landlords_mpan: '1234567890123'" redacts
     # as <my_landlords_mpan> rather than every entry collapsing into one indistinguishable label.
-    "redact_strings_labelled": {"type": "dict"},
+    "redact_strings_labelled": {"type": "dict", "scalar_value_dict": True},
 }

apps/predbat/hass.py

@@ -185,6 +185,16 @@ class Hass:
     # 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_pattern(self):
         """
         Return the cached compiled redaction pattern log() must apply, rebuilding it the first
@@ -194,14 +204,24 @@ def _log_secret_pattern(self):
         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.
         """
-        if self._log_secret_pattern_cache is self._LOG_SECRET_PATTERN_UNSET:
-            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)
-        return self._log_secret_pattern_cache
+        with self._log_secret_pattern_lock:
+            if self._log_secret_pattern_cache is self._LOG_SECRET_PATTERN_UNSET:
+                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)
+            return self._log_secret_pattern_cache
 
     def log(self, msg, quiet=True):
         """
@@ -302,10 +322,22 @@ def load_secrets(self):
             self.log(f"Loading secrets from {secrets_file}", quiet=False)
             try:
                 with io.open(secrets_file, "r") as stream:
-                    secrets = yaml.safe_load(stream) or {}
-                    # Check for debug logging option
-                    if secrets.get("logger") == "debug":
-                        self.log(f"Info: Secrets loaded from {secrets_file}", quiet=False)
+                    loaded = yaml.safe_load(stream) or {}
+                    if not isinstance(loaded, dict):
+                        # Valid YAML (a bare scalar or list at the top level) but the wrong shape -
+                        # yaml.safe_load() raises nothing here, so without this check `secrets`
+                        # below would be reassigned to that scalar/list before the .get() call two
+                        # lines down throws AttributeError. The generic except then logs the crash
+                        # but the reassignment has already happened and is never undone, so
+                        # load_secrets() still returns the malformed value - and the very next
+                        # log() call reaches collect_log_secret_values()'s secrets.items(), which
+                        # raises unhandled and aborts startup entirely (#5053 review).
+                        self.log(f"Error: secrets.yaml at {secrets_file} must be a mapping of name: value, found {type(loaded).__name__} - ignoring it", quiet=False)
+                    else:
+                        secrets = loaded
+                        # Check for debug logging option
+                        if secrets.get("logger") == "debug":
+                            self.log(f"Info: Secrets loaded from {secrets_file}", quiet=False)
             except yaml.YAMLError as exc:
                 self.log(f"Error: Failed to load secrets from {secrets_file}: {exc}", quiet=False)
             except Exception as exc:
@@ -335,13 +367,14 @@ 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.logfile = open("predbat.log", "a")
 
         # Load secrets first
         self.secrets = self.load_secrets()
-        self._log_secret_pattern_cache = self._LOG_SECRET_PATTERN_UNSET
+        self._invalidate_log_secret_pattern()
 
         # Register custom YAML constructor for !secret tag
         yaml.add_constructor("!secret", self.secret_constructor, Loader=yaml.SafeLoader)
@@ -353,7 +386,7 @@ def __init__(self):
             try:
                 config = yaml.safe_load(stream)
                 self.args = config["pred_bat"]
-                self._log_secret_pattern_cache = self._LOG_SECRET_PATTERN_UNSET
+                self._invalidate_log_secret_pattern()
             except yaml.YAMLError as exc:
                 print(exc)
                 sys.exit(1)

apps/predbat/predbat.py

@@ -1641,6 +1641,18 @@ def validate_config(self):
                                     self.arg_errors[name] = "Invalid type, element {} expected dict".format(item)
                                     errors += 1
                                     break
+
+                                # scalar_value_dict (e.g. redact_strings_labelled): a mapping value
+                                # that isn't already a string passes this branch silently since it
+                                # only checks item is a dict, but the consumer collecting these
+                                # values only matches strings - an unquoted numeric MPAN/account ID
+                                # loads from YAML as an int and would otherwise go unredacted with
+                                # no warning at all (#5053 review). Warn, don't error: the value is
+                                # still usable once coerced to a string by the collector.
+                                if spec.get("scalar_value_dict", False):
+                                    for key, sub_value in item.items():
+                                        if not isinstance(sub_value, str):
+                                            self.log("Warn: Validation of apps.yaml found configuration item '{}' entry '{}' value {} is not a string - quote it to ensure it is matched".format(name, key, sub_value))
                     elif expected_type == "int_float_dict":
                         if spec.get("or_auto", False) and value == "auto":
                             matches = True

apps/predbat/tests/test_chat_tools.py

@@ -28,7 +28,7 @@
 import tempfile
 
 import chat_tools
-from utils import mask_secret_args, parse_yaml_path
+from utils import mask_secret_args, parse_yaml_path, compile_log_secret_pattern
 from chat_tools import CHAT_TOOL_DEFS, DOCS_READ_MAX_CHARS, read_docs, score_documents, search_docs, read_source, search_source, resolve_source_path, SourceAccessError, is_endpoint_key
 from chat_tools import DEFAULT_FETCH_ALLOWLIST, FetchRefusedError, host_allowed, html_to_text, validate_fetch_target
 from chat_tools import APPS_YAML_RESTART_WARNING, set_apps_config, validate_apps_schema_type
@@ -922,6 +922,44 @@ def test_set_apps_config_success_preserves_formatting_backs_up_and_mirrors_args(
     return failed
 
 
+def test_set_apps_config_success_invalidates_log_secret_cache(my_predbat):
+    """A successful write must invalidate log()'s cached redaction pattern (#5053 review).
+
+    set_apps_config() writes straight to base.args (base.args[key] = value) rather than going
+    through set_arg() or the web editor's batch apply - both of which already invalidate the
+    cache - so it was its own separate gap: a credential-adjacent value saved through the chat
+    agent's tool would keep leaking into the log under the stale pattern until restart, the same
+    class of bug GH#4770 exists to close.
+    """
+    failed = False
+    print("**** Testing set_apps_config invalidates the cached log redaction pattern ****")
+    root = tempfile.mkdtemp(prefix="predbat_apps_")
+    original_args_value = my_predbat.args.get("num_inverters")
+    saved_cache = my_predbat._log_secret_pattern_cache
+    try:
+        apps_path = _apps_yaml_fixture(root)
+        backup_path = apps_path + ".backup"
+        my_predbat.args["num_inverters"] = 1
+        # A stale value built the real way, not an arbitrary sentinel - matches the convention in
+        # test_web_apps_edit.py's equivalent cache-invalidation test.
+        stale_cache_marker = compile_log_secret_pattern({"stale-pattern-marker-5053xx": "marker"})
+        my_predbat._log_secret_pattern_cache = stale_cache_marker
+
+        result = set_apps_config(my_predbat, "num_inverters", 2, apps_yaml_path=apps_path, backup_path=backup_path)
+
+        if not result.get("success"):
+            print("ERROR: a valid change was refused: {}".format(result))
+            return True
+        if my_predbat._log_secret_pattern_cache is stale_cache_marker:
+            print("ERROR: a successful set_apps_config write left the cached log redaction pattern stale")
+            failed = True
+    finally:
+        my_predbat.args["num_inverters"] = original_args_value
+        my_predbat._log_secret_pattern_cache = saved_cache
+        shutil.rmtree(root, ignore_errors=True)
+    return failed
+
+
 def test_validate_apps_schema_type(my_predbat):
     """validate_apps_schema_type checks a value's shape against APPS_SCHEMA, permissively for a
     key with no schema entry at all - not every apps.yaml key is declared there."""
@@ -1385,5 +1423,6 @@ def run_chat_tools_tests(my_predbat):
     failed |= test_set_apps_config_refuses_unknown_key(my_predbat)
     failed |= test_set_apps_config_refuses_a_schema_type_mismatch(my_predbat)
     failed |= test_set_apps_config_success_preserves_formatting_backs_up_and_mirrors_args(my_predbat)
+    failed |= test_set_apps_config_success_invalidates_log_secret_cache(my_predbat)
     failed |= test_validate_apps_schema_type(my_predbat)
     return failed