Skip to content

Commit 2dd5a06

Browse files
fix(security): address third-round Copilot review on #5053 - stale warning text, args aliasing, a key.lower() crash, numeric redact_strings, and overlap labels
Five suppressed findings from the latest Copilot review of the #4770 write-time log redaction PR, all confirmed and fixed. Warning text contradicted the actual (already-fixed) behaviour ------------------------------------------------------------------ The scalar_value_dict warning told the user to quote a numeric value "to ensure it is matched", but collect_log_secret_values() already coerces it to a string regardless. Reworded to explain the real reason to quote it: keeping exact formatting (a leading zero, say) rather than losing it to numeric parsing. A test restored my_predbat.args by reassignment, not in place ------------------------------------------------------------------ ComponentBase.__init__ aliases self.args = base.args at construction, so a test that restores my_predbat.args = saved_args (rather than mutating the same dict object back to its original contents) 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. Fixed both _run()'s shared restore and the redact_strings_labelled numeric-value test's own restore to clear()+update() in place. Confirmed the leak directly: with the old reassignment restore, a stand-in "component alias" dict still carried the test's added key after _run() returned; with the fix, it does not. _collect_secret_values crashed on a non-string top-level dict key ------------------------------------------------------------------ The redact_strings/redact_strings_labelled exemption added for the label-priority fix called key.lower() unconditionally. is_secret_key() already tolerates a non-string key (str(key).lower()) for exactly this reason - log() runs on the very first startup line, before validation has had any chance to report a malformed apps.yaml. Matched the same guard. Numeric redact_strings (bare list) values were still silently dropped ------------------------------------------------------------------------ The earlier fix coerced scalar secrets.yaml and redact_strings_labelled values to string but missed the parallel redact_strings list form - an unquoted numeric MPAN there (redact_strings: [1234567890123]) still failed the isinstance(value, str) check and never reached the log redaction set. An overlapping-secret mask lost its label ------------------------------------------------------------------------ redact_log_line()'s overlap-extension fix (previous commit) covered the full extended span but looked its label up as labels.get(merged_span, ...) - the merged span spanning more than one secret is not itself a key in labels, so it silently fell back to the generic <xxx> mask, losing the "which credential" guarantee a labelled mask exists to provide even though nothing leaked. Now tracks the label of every secret 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 failure its regression 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>
1 parent d3585d6 commit 2dd5a06

4 files changed

Lines changed: 66 additions & 11 deletions

File tree

‎apps/predbat/predbat.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1652,7 +1652,7 @@ def validate_config(self):
16521652
if spec.get("scalar_value_dict", False):
16531653
for key, sub_value in item.items():
16541654
if not isinstance(sub_value, str):
1655-
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))
1655+
self.log("Warn: Validation of apps.yaml found configuration item '{}' entry '{}' value {} is not a string - quote it to keep its exact formatting (e.g. a leading zero)".format(name, key, sub_value))
16561656
elif expected_type == "int_float_dict":
16571657
if spec.get("or_auto", False) and value == "auto":
16581658
matches = True

‎apps/predbat/tests/test_secrets.py‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,27 @@ def test_collect_log_secret_values():
323323
print("ERROR: a boolean secrets.yaml value should not be collected, got {}".format(bool_found))
324324
failed = True
325325

326+
# An unquoted numeric MPAN in the bare redact_strings list form (not just the labelled dict
327+
# form) loads from YAML as an int and must still be collected, coerced to string - the same
328+
# gap as the labelled form, just missed on the first pass (#5053 review).
329+
numeric_bare_found = collect_log_secret_values({}, {}, [1234567890123], None)
330+
if "1234567890123" not in numeric_bare_found or numeric_bare_found["1234567890123"] != "redact_strings":
331+
print("ERROR: a numeric redact_strings (bare list) value should be collected as a string, got {}".format(numeric_bare_found))
332+
failed = True
333+
334+
# A non-string dict key (an integer key in a nested mapping, reached at the true top level of
335+
# args) must not crash the traversal - log() runs on the very first startup line, before
336+
# validation has had any chance to report the malformed input (#5053 review).
337+
try:
338+
non_string_key_found = collect_log_secret_values({123: "some_value"}, {})
339+
except AttributeError as e:
340+
print("ERROR: a non-string dict key crashed the traversal instead of being tolerated: {}".format(e))
341+
failed = True
342+
non_string_key_found = {}
343+
if non_string_key_found:
344+
print("ERROR: a non-secret-flagged non-string key should not itself be collected, got {}".format(non_string_key_found))
345+
failed = True
346+
326347
if not failed:
327348
print("**** test_collect_log_secret_values PASSED ****")
328349
return failed
@@ -388,15 +409,25 @@ def test_compile_log_secret_pattern_and_redact_log_line():
388409
if "sec1" in diff_position_redacted or "c123x" in diff_position_redacted:
389410
print("ERROR: an overlapping secret value survived redaction: {}".format(diff_position_redacted))
390411
failed = True
412+
# The merged span itself is not a key in `labels` (it is neither secret value alone), so a
413+
# naive labels.get(merged_value, SECRET_MASK) falls back to the generic mask - losing the
414+
# "which credential" guarantee a labelled mask exists to provide, even though nothing leaks
415+
# (Copilot review on #5053). Both contributing labels must survive, joined together.
416+
if "<short_secret+long_secret>" not in diff_position_redacted:
417+
print("ERROR: an overlapping match should keep both labels, got {}".format(diff_position_redacted))
418+
failed = True
391419

392420
# A three-way overlap chain (v1/1v2/v2345 all present in "v1v2345") must extend through
393-
# every overlap, not just the first one found.
421+
# every overlap, not just the first one found - and keep every contributing label.
394422
chain_found = {"v1": "l1", "1v2": "l2", "v2345": "l3"}
395423
chain_pattern = compile_log_secret_pattern(chain_found)
396424
chain_redacted = redact_log_line("value v1v2345 end", chain_pattern)
397425
if "v1v2345" in chain_redacted or "2345" in chain_redacted or "v2345" in chain_redacted:
398426
print("ERROR: a chained overlap was not fully covered: {}".format(chain_redacted))
399427
failed = True
428+
if "<l1+l2+l3>" not in chain_redacted:
429+
print("ERROR: a chained overlap should keep every contributing label, got {}".format(chain_redacted))
430+
failed = True
400431

401432
# A line with nothing secret in it must come back byte-identical.
402433
clean_line = "Info: battery_size is 9.5 today"

‎apps/predbat/tests/test_validate_config.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,12 @@ def _run(my_predbat, extra_args, extra_states=None, expect_errors=(), expect_cle
5353
for name in expect_clean:
5454
assert name not in my_predbat.arg_errors, f"Unexpected validation error for '{name}': {my_predbat.arg_errors.get(name)}"
5555
finally:
56-
my_predbat.args = saved_args
56+
# In place, not my_predbat.args = saved_args - a ComponentBase aliases self.args = base.args
57+
# at construction time (component_base.py), so reassigning here would leave any component
58+
# already holding that reference still pointing at the old, now-extra_args-mutated dict
59+
# even though my_predbat.args itself looks restored (#5053 review).
60+
my_predbat.args.clear()
61+
my_predbat.args.update(saved_args)
5762
my_predbat.ha_interface.dummy_items = saved_states
5863

5964

@@ -213,7 +218,9 @@ def test_validate_config(my_predbat):
213218
assert "redact_strings_labelled" not in my_predbat.arg_errors, "a numeric redact_strings_labelled value should warn, not error: {}".format(my_predbat.arg_errors.get("redact_strings_labelled"))
214219
assert any("my_mpan" in msg and "not a string" in msg for msg in captured), "expected a 'not a string' warning naming my_mpan, got {}".format(captured)
215220
finally:
216-
my_predbat.args = saved_args
221+
# In place, not reassignment - see _run()'s equivalent comment above (#5053 review).
222+
my_predbat.args.clear()
223+
my_predbat.args.update(saved_args)
217224
my_predbat.log = saved_log
218225

219226
# ==========================================================================

‎apps/predbat/utils.py‎

Lines changed: 24 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,11 @@ def _collect_secret_values(value, found, label_prefix=""):
256256
# the more specific one redact_strings_labelled would have given it (#5053 review).
257257
# Only exempt the true top-level keys (label_prefix empty) - an unrelated nested key
258258
# that happens to share the name is not these denylists and should still collect.
259-
if not label_prefix and key.lower() in SECRET_KEY_EXPLICIT_NAMES:
259+
# str(key) first: apps.yaml keys are always strings in practice, but is_secret_key()
260+
# below already tolerates a non-string key the same way, and log() runs on the very
261+
# first startup line - an unguarded .lower() here would crash before validation ever
262+
# gets a chance to report the malformed input (#5053 review).
263+
if not label_prefix and str(key).lower() in SECRET_KEY_EXPLICIT_NAMES:
260264
continue
261265
if is_secret_key(key):
262266
key_label = (label_prefix + "." + key) if label_prefix else key
@@ -350,8 +354,10 @@ def collect_log_secret_values(args, secrets, redact_strings=None, redact_strings
350354
found[value] = str(label)
351355
if isinstance(redact_strings, list):
352356
for value in redact_strings:
353-
if isinstance(value, str) and value and value not in found:
354-
found[value] = "redact_strings"
357+
if isinstance(value, (str, int, float)) and not isinstance(value, bool):
358+
value = str(value)
359+
if value and value not in found:
360+
found[value] = "redact_strings"
355361
return found
356362

357363

@@ -399,7 +405,10 @@ def redact_log_line(line, secret_pattern):
399405
matches at the earliest position ("sec1"), then resumes scanning after it, so it never
400406
considers "c123x" starting one character in and leaves "23x" exposed. Extend each match to
401407
the longest secret that starts anywhere inside it before emitting the mask, so a longer
402-
secret overlapping a shorter one is always fully covered.
408+
secret overlapping a shorter one is always fully covered. Every secret that contributed to an
409+
extended span keeps its own label in the mask (joined with "+"), rather than falling back to
410+
the generic mask just because the merged span itself is not a single known value (#5053
411+
review) - the point of a labelled mask is telling an operator which credential to check.
403412
"""
404413
if secret_pattern is None or not line:
405414
return line
@@ -411,7 +420,13 @@ def redact_log_line(line, secret_pattern):
411420
if start < pos:
412421
# Already covered by the extended span of a previous match.
413422
continue
414-
value = match.group(0)
423+
# Collect the label of every secret found to contribute to the (possibly extended) span,
424+
# in the order encountered, rather than looking up labels[line[start:end]] once at the
425+
# end - a merged span covering more than one overlapping secret is not itself a key in
426+
# labels, so that lookup would silently fall back to the generic mask and the line would
427+
# read no differently from an unrecognised value, losing the "which credential" guarantee
428+
# this feature exists to provide (#5053 review).
429+
span_labels = [labels.get(match.group(0), SECRET_MASK)]
415430
# Look for a longer secret starting at each position within this match's span and extend
416431
# to cover it - finditer() itself won't report an overlapping match once it has already
417432
# consumed the earlier one, so each candidate start position must be probed directly with
@@ -423,10 +438,12 @@ def redact_log_line(line, secret_pattern):
423438
rescan = pattern.match(line, probe)
424439
if rescan and rescan.end() > end:
425440
end = rescan.end()
426-
value = line[start:end]
441+
rescan_label = labels.get(rescan.group(0), SECRET_MASK)
442+
if rescan_label not in span_labels:
443+
span_labels.append(rescan_label)
427444
extended = True
428445
out.append(line[pos:start])
429-
out.append("<{}>".format(labels.get(value, SECRET_MASK)))
446+
out.append("<{}>".format("+".join(span_labels)))
430447
pos = end
431448
out.append(line[pos:])
432449
return "".join(out)

0 commit comments

Comments
 (0)