fix(security): redact secrets at log-write time, not just at serve time (#4770)

[chalfontchubby]

Posted by Claude on behalf of @chalfontchubby.

Addresses #4770.

Problem

Every existing masking path (create_debug_yaml(), mask_secret_yaml_text(), web.py's apps.yaml route) only redacts at serve/download time. Some users copy predbat.log directly off a Samba share exposing the addon's config directory, bypassing every HTTP/MCP endpoint entirely - a scrub applied only at those endpoints leaves the on-disk file itself holding the plaintext credential.

Separately, annual.py's scrub_secrets() was an independent implementation with its own hardcoded 4-substring credential list, disconnected from the growable per-component "secret": True registry the shared utils.py machinery has since grown (account numbers, serial numbers, etc). Growing that registry did not benefit this path.

Predbat also has no way to know an MPAN, account number, or similar identifier surfaced by a third-party HA integration's entity state/attributes is sensitive - there is no schema for arbitrary third-party data, so nothing could redact it even in principle.

Fix

Write-time log redaction. Hass.log() now redacts every known secret value before the line is written to predbat.log, closing the Samba-share gap. Implementation notes:

• A single compiled regex alternation (not a per-value str.replace() loop), cached and rebuilt only when args/secrets actually change - the hot logging path pays for one scan per line regardless of how many secrets are configured.
• Values are matched longest-first, so a secret sharing a prefix with a longer one can't pre-empt the more specific match and leave part of it exposed.
• A masked occurrence carries a label identifying which credential was found - <octopus_api_key>, not one opaque xxx indistinguishable from every other secret - so a log stays diagnosable without ever writing out the value itself.

User-maintained redaction denylist, for anything Predbat cannot recognise as a credential on its own:

• redact_strings - a plain list of values, masked generically as <redact_strings>.
• redact_strings_labelled - a {label: value} mapping, so a user's own entry gets a real identifying label back too, the same way a built-in credential is labelled by its config key name.

Both are masked wholesale in a debug dump - for the labelled form, the user's own label names are hidden too, since a key like landlord_mpan can be as informative as the value it names.

annual.py's scrub_secrets() now delegates to the shared utils.mask_secret_args() instead of keeping its own list, so it inherits every future registry addition automatically.

Deliberately out of scope: user-authored regex patterns for redaction (e.g. a template for "looks like an MPAN"). That opens real questions of its own - pattern validation, ReDoS risk from a pattern compiled into the hot logging path, whether a regex hit is safe to label - better left for its own discussion than folded into this fix.

Verification

Mutation-tested: reverting the write-time call in log() causes every credential, redact_strings and redact_strings_labelled test to fail with the exact expected message; restoring it passes.

• ./run_all --quick green (310 tests)
• ./run_pre_commit green

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Unresolved redaction issues could expose secrets or prevent startup.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds write-time secret redaction for logs, configurable redaction denylists, and shared masking for annual reports.

Changes:

• Adds cached, labelled regex-based log redaction.
• Adds redact_strings and redact_strings_labelled.
• Extends tests and reuses shared masking in annual.py.

File summaries

File	Summary and final findings
apps/predbat/utils.py	Secret collection and masking. Critical (2 votes): secret-valued lists are not collected. Moderate (3 votes): short explicit secrets are not redacted. Moderate (2 votes): invalid labelled values can break startup. Moderate (1 vote): numeric labelled values are discarded. Moderate (1 vote): numeric YAML secrets are skipped.
apps/predbat/tests/test_validate_config.py	Configuration validation tests; no final comments.
apps/predbat/tests/test_secrets.py	Redaction tests. Moderate (2 votes): PREDBAT_APPS_FILE is deleted instead of restoring its prior value.
apps/predbat/hass.py	Write-time log redaction. Critical (2 votes): the cache is not invalidated after in-place argument or secret changes.
apps/predbat/config.py	Denylist schema. Moderate (1 vote): non-string labelled values pass validation but are ignored by collection.
apps/predbat/annual.py	Uses shared secret masking; no final comments.

Review details

Suppressed comments (3)

apps/predbat/config.py:2809

• The new labelled denylist accepts any dict value, but the collector silently ignores non-string values. A natural my_mpan: 1234567890123 therefore passes this schema and is never redacted. Validate mapping values as strings or collect scalar values consistently.

    "redact_strings_labelled": {"type": "dict"},

apps/predbat/utils.py:305

• redact_strings_labelled is accepted as any dict by APPS_SCHEMA, so an unquoted numeric MPAN such as {landlord_mpan: 1234567890123} passes validation. YAML loads that value as an integer, but this isinstance(value, str) check discards it, so the denylist fails for a common form of the identifier and the log can expose it. Normalize scalar mapping values to strings or validate the mapping as string-valued before collecting it.

    if redact_strings_labelled:
        for label, value in redact_strings_labelled.items():
            if isinstance(value, str) and len(value) >= 6 and value not in found:
                found[value] = str(label)

apps/predbat/utils.py:295

• Secrets loaded from YAML are not guaranteed to be strings. An unquoted numeric MPAN/account ID (or numeric !secret) is returned as an int and is skipped by this isinstance(value, str) check, even though log() serializes the value with str(msg). That leaves a known identifier unredacted; normalize supported scalar values or fail closed.

    if secrets:
        for key, value in secrets.items():
            if isinstance(value, str) and len(value) >= 6 and value not in found:
                found[value] = key

• Files reviewed: 6/6 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved redaction correctness, cache synchronization, and configuration-handling issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

apps/predbat/hass.py:203

• This cache is still bypassed by in-place configuration writes outside the two invalidation sites: web_chat.py:705 assigns a provider block containing nested api_key, and chat_tools.py:773 writes directly to base.args. After the first log builds the pattern, either path can add or change a credential while leaving this old pattern active, so the new value can be written to predbat.log in plaintext until restart. Route all config mutations through one invalidating helper or refresh from a configuration generation/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)

apps/predbat/utils.py:249

• SECRET_KEY_EXPLICIT_NAMES makes redact_strings get collected during this generic args traversal, before the dedicated labelled-denylist pass at line 326. If the same value is present in both denylist forms (or under a credential key later in args), the generic <redact_strings> label wins based on YAML insertion order, contrary to the documented rule that a specific label wins. Skip the explicit denylist keys here and collect them only in the ordered blocks below.

        for key, item in value.items():
            if is_secret_key(key):

• Files reviewed: 11/11 changed files
• Comments generated: 5
• Review effort level: Lite

[chalfontchubby]

(Posted by Rik; written by Claude.)

Addressed the two suppressed comments from the latest review, plus the 5 inline-thread findings from the earlier one (replied/resolved individually on their threads):

web_chat.py:705 / chat_tools.py:773 - missed cache invalidation sites: both wrote straight to base.args (a provider block that can carry a nested api_key, and an arbitrary set_apps_config value respectively) without invalidating the cached log redaction pattern, so a credential saved through either path would keep leaking into the log under the stale pattern until restart. Both now call the shared _invalidate_log_secret_pattern() helper.

hass.py:249 - SECRET_KEY_EXPLICIT_NAMES label-priority ordering: while fixing the cache invalidation gaps I also found and fixed the ordering issue this comment flagged. redact_strings/redact_strings_labelled are secret-flagged (via SECRET_KEY_EXPLICIT_NAMES in is_secret_key()) so the debug-dump masking hides them wholesale - but that same flag made the generic args traversal 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 "a more specific label wins" priority. A value present in both denylist forms kept the generic <redact_strings> label instead of the more specific one. Fixed by skipping the two true top-level keys in that traversal (an unrelated nested key merely sharing the name still collects normally, labelled by its own key path).

All fixes verified with mutation testing (reverting each one in turn reproduces exactly the predicted failure, caught by a new/extended test) and the full ./run_all --quick + ./run_pre_commit are green. Pushed as d3585d65.

[Copilot]

🟡 Changes recommended

Unresolved critical redaction gaps and concurrency issues could still expose sensitive values.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (5)

apps/predbat/predbat.py:1655

• The collector now explicitly coerces numeric labelled values to strings, so an unquoted numeric denylist value is still matched. This warning says quoting is required “to ensure it is matched,” which contradicts the implemented behavior and the new numeric-value handling; revise it to explain coercion (or the separate risk of losing formatting such as leading zeros).

                                            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))

apps/predbat/tests/test_validate_config.py:216

• Assigning a new dict here leaves initialized components holding the old base.args object (component_base.py:63-70). This test updates that old object with redact_strings_labelled, so subsequent tests can observe different configuration through component aliases even though my_predbat.args looks restored. Restore the shared mapping in place instead of replacing it.

        my_predbat.args = saved_args

apps/predbat/utils.py:262

• This traversal assumes every YAML mapping key is a string. A valid nested mapping with an integer or other non-string key reaches key.lower() during the first startup log and raises before configuration validation can report the input, potentially aborting startup. Normalize the key before both the explicit-name check and label construction.

            if not label_prefix and key.lower() in SECRET_KEY_EXPLICIT_NAMES:
                continue
            if is_secret_key(key):
                key_label = (label_prefix + "." + key) if label_prefix else key

apps/predbat/utils.py:354

• A numeric entry in the explicitly user-maintained redact_strings list is ignored here. MPANs and account identifiers are commonly written as unquoted YAML numbers, so the denylist can appear configured while the corresponding value still reaches predbat.log in plaintext (and may be printed by validation). Coerce non-boolean numeric entries just as the labelled denylist does.

    if isinstance(redact_strings, list):
        for value in redact_strings:
            if isinstance(value, str) and value and value not in found:
                found[value] = "redact_strings"

apps/predbat/utils.py:429

• For overlaps that start at different offsets (and for the chained-overlap case covered by the new tests), value is rebuilt as the union of several secret strings and is not a key in labels. The fallback therefore emits <xxx>, losing the labelled-redaction guarantee described by this change and making the affected log line indistinguishable from an unlabelled mask. Preserve the label(s) of the extending matches, or explicitly define a labelled representation for a multi-secret overlap.

        out.append("<{}>".format(labels.get(value, SECRET_MASK)))

• Files reviewed: 16/16 changed files
• Comments generated: 12
• Review effort level: Lite

[chalfontchubby]

(Posted by Rik; written by Claude.)

Addressed the 5 suppressed comments from the latest review:

predbat.py:1655 - stale warning text: the scalar_value_dict warning said quoting a numeric value was needed "to ensure it is matched" - but the collector already coerces it to a string regardless, so that's no longer true. Reworded to explain the real reason to quote it (keeping exact formatting, e.g. a leading zero, rather than losing it to numeric parsing).

test_validate_config.py:216 - args restored by reassignment, not in place: ComponentBase.__init__ aliases self.args = base.args at construction, so restoring via my_predbat.args = saved_args leaves any component already holding that reference pointing at the old, still-mutated dict - the test itself looks clean but a component alias is not. Confirmed the leak directly with a stand-in alias, then fixed both _run()'s shared restore (used by every test in the file) and this test's own restore to clear()+update() in place.

utils.py:262 - key.lower() crash on a non-string key: the label-priority exemption added last round called .lower() unconditionally; is_secret_key() already guards this exact case (str(key).lower()) since log() runs on the very first startup line, before validation gets a chance to report a malformed apps.yaml. Matched the same guard.

utils.py:354 - numeric redact_strings (bare list) still dropped: the earlier numeric-coercion fix covered secrets.yaml and redact_strings_labelled but missed the parallel bare-list form - an unquoted numeric MPAN there still failed isinstance(value, str) and never reached the log redaction set.

utils.py:429 - an overlapping-secret mask lost its label: the overlap-extension fix covered the value correctly (nothing leaks) but looked its label up as labels.get(merged_span, ...) - a span covering more than one overlapping secret isn't itself a key in labels, so it silently fell back to the generic <xxx> mask, losing the "which credential" guarantee. Now tracks every secret's label that contributed to the (possibly chained) extended span and joins them, e.g. <api_key+redact_strings>.

All five verified with mutation testing (reverting each fix in turn reproduces exactly the predicted failure, caught by a new/extended test). Full ./run_all --quick + ./run_pre_commit green. Pushed as 2dd5a066.

[Copilot]

🟡 Changes recommended

Critical redaction gaps and race conditions remain, along with moderate input-handling and test-isolation issues.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (8)

apps/predbat/tests/test_secrets.py:528

• This test also unconditionally deletes the shared predbat.log after closing only its temporary Hass handle; the shared my_predbat.logfile remains open for all subsequent registry tests. That makes the full suite order-dependent and can remove a caller's existing log. Use a temporary working directory or restore the original file rather than removing this path.

        for name in ("test_apps.yaml", "secrets.yaml", "predbat.log"):
            if os.path.exists(name):
                os.remove(name)

apps/predbat/tests/test_secrets.py:131

• This cleanup unlinks the shared predbat.log while the single my_predbat used by the test registry still has its logfile open. Later tests keep writing to the unlinked handle, so log readers see a missing/stale file and any pre-existing log is destroyed. Run the temporary Hass instance in an isolated directory or preserve and restore the original log instead of deleting it.

        if os.path.exists("predbat.log"):
            os.remove("predbat.log")

apps/predbat/tests/test_web_chat.py:2947

• This new test changes the shared my_predbat.args["chat"] through the route, but the finally block restores only the cache and module path. The unit suite reuses one PredBat instance; restoring the pre-save cache while leaving sk-or-new-secret in args makes the next tests run with an inconsistent redaction state and can leak this fixture value. Save and restore the prior chat block as well, then restore/invalidate the cache consistently.

    finally:
        web_chat.APPS_YAML_PATH = original
        my_predbat._log_secret_pattern_cache = saved_cache

apps/predbat/userinterface.py:212

• This invalidates only after mutating self.args. A concurrent log() can enter between the assignment and _invalidate_log_secret_pattern(), reuse the old compiled pattern, and write the newly added credential before the invalidation acquires the lock; the lock only ensures the stale cache is removed afterward. Make each mutation plus cache-generation update atomic (the same sequence exists in the web/chat writers), and cover this interleaving in the race test.

        # 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
        # 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()

apps/predbat/utils.py:267

• Registry-marked credentials can still be numeric when an apps.yaml value is unquoted (for example an MPAN/account ID). This branch only collects strings, so collect_log_secret_values({"kraken_mpan": 1234567890123}, {}) produces no pattern and a subsequent log can write the identifier verbatim. Coerce scalar int/float values to strings here, as the explicit denylist sources already do.

                if isinstance(item, str) and item and item not in found:

apps/predbat/utils.py:248

• The comments say malformed non-string keys are tolerated, but raw key is later concatenated with strings for nested prefixes and can also be stored as a non-string label. A YAML mapping with a numeric nested key can therefore raise during the first log, and a non-string secret label can make '+'.join(span_labels) fail. Normalize the key once at the loop boundary.

        for key, item in value.items():

apps/predbat/utils.py:354

• The labelled denylist key is copied verbatim into the replacement written to the log. A valid configuration such as redact_strings_labelled: {secret: secret} therefore produces <secret> and leaks the denylisted value; labels containing newlines can also inject log lines. Validate/sanitize labels or fall back to a generic mask whenever a label contains secret/control content.

                    found[value] = str(label)

apps/predbat/utils.py:202

• Because this check runs before the registry switch, is_secret_key(key, registry=False) no longer has the documented “substring-only” meaning. In particular, check_apps_yaml_secrets() now flags the whole redact_strings list/mapping as an inline credential even when its entries use the documented !secret form, since the raw-file walker does not recurse once the container key is treated as secret. Keep denylist masking separate from the credential heuristic or make the raw checker recurse these containers.

    if key_lower in SECRET_KEY_EXPLICIT_NAMES:
        return True

• Files reviewed: 16/16 changed files
• Comments generated: 5
• Review effort level: Lite

[chalfontchubby]

(Written by Claude, working with @chalfontchubby.)

Sixth round addressed structurally rather than hole-by-hole, because reading the findings together they were all one shape: two call sites bypassing helpers this PR already has.

The core is one predicate (is_secret_key), one recursive traversal (mask_secret_args), one write-back guard (find_redacted_secret_overwrite). Nine of ten consumers use them and are correct by construction. The two that hand-rolled flat versions produced all six comments — so the bypasses are gone, not their edge cases patched:

• /apps rendering masked only top-level-named credentials, then handed the raw structure to render_type(), which recurses. Now masked once at the route through mask_secret_args(), so nested credentials are covered as new ones appear.
• /apps writing gets the guard that already protects the model's write path, so a masked row saved back is refused rather than destroying the key.
• The denylist tails dropped any non-scalar shape silently while validation only warned. Both now flatten through one normaliser.
• str(key) in the traversal, at both sites rather than only the reported one.

Worth stating what this already closes, since the review has been running hole-to-hole: every nested credential on the /apps page, and every non-scalar denylist entry — two whole classes, not six instances. forecast_solar[0].api_key was the documented case mask_secret_args was written for, and the page was the one surface not asking it.

Three things to push back on deliberately, rather than leave to be found:

1. Over-collection is now the posture. _flatten_denylist_value() str()s even unknown types. Asymmetric failure modes: over-collecting redacts a harmless word, under-collecting leaks a credential.
2. One test changed sides. It asserted redact_strings={"not": "a list"} collects nothing; it now collects the value inside. The test's own comment already argued that for bare strings and merely carved out dicts — and redact_strings: {mpan: 123…} is exactly the plausible mix-up with redact_strings_labelled that was being dropped. Reasoning is recorded in the test.
3. A real behaviour change: editing a masked credential row on /apps is now refused. That is what makes masking safe, but it is new surface — say if it should move.

The one remaining leak is filed as #5106 with a TODO at the site: resolve_arg_re() logs a matched entity id before redaction covers it. Deferred because the value is an entity id rather than a resolved credential, and the fix needs a decision about where that diagnostic belongs rather than a reflex patch.

Tests: 3 new /apps cases (nested masking deep-copies, refusal on both a dict and a list path, rotation still works), 3 new collector cases (malformed shape, double-nested, None/bool/empty dropped), 1 new traversal case (credential under an int key, asserting the outer.123.password label). Pre-commit clean, quick suite green.