Files
LEDMatrix/test/web_interface/test_secret_helpers.py
T
ChuckBuildsandClaude Opus 5 d64fa9761a Stop array-item secrets being wiped, and logging them
Three review findings from #485 that I missed when addressing that PR;
it has since merged, so they land here.

1. Array-item secrets destroyed by any unrelated save (data loss).

remove_empty_secrets recursed into dicts but let a list fall through to
the scalar branch and kept it verbatim. Lists merge by *replacement*, so
the blanks the masked form posts back went straight over the stored
array:

    stored   [{"name":"a","token":"REAL-A"}, {"name":"b","token":"REAL-B"}]
    posted   [{"name":"a","token":""},       {"name":"b","token":""}]
    merged   [{"name":"a","token":""},       {"name":"b","token":""}]
             -> both credentials gone

Same failure as the scalar api_key case fixed earlier, one container
deeper. Lists now prune element-wise, and a list with nothing real in it
is dropped so the stored one is left alone. Where one entry does change,
the new merge_secrets merges by index instead of replacing.

Two details the first attempt got wrong, both caught by existing tests:

- An emptied dict item must stay {}, not None. ConfigManager's
  _strip_secrets_recursive treats a secrets list as *parallel* to the
  regular one ({} = "item i has no secrets"); a None makes it stop
  looking parallel, and it then drops the whole key from the main config
  -- silently deleting the items' non-secret fields too.
- The incoming list's length wins. The regular config's list is
  authoritative about how many items exist, so preserving surplus stored
  entries would let the two fall out of step and make deleting an entry
  impossible.

2. Submitted credentials written to the journal (security).

save_plugin_config logged `Full config: {plugin_config}` at INFO and
`Config that failed: {plugin_config}` at ERROR. Both run before
separate_secrets, so plugin_config still held the values just typed into
the form. Now keys only. Swept the rest of web_interface/ and src/ for
the same shape -- these were the only two.

3. Restart banner kept stale wording.

showRestartPending() cleared the stored custom text but left the DOM
element alone, so a config save could show the previous update's
message. The default is read back from the server-rendered copy rather
than duplicated in JS, so the template stays the one owner of the string.

Verified: 556 passed, 1 skipped across the web suite. Mutation-checked --
reverting api_v3 fails the logging guard and the array-merge test;
reverting either half of the secret_helpers change fails the unit tests.
New end-to-end coverage drives the real endpoint, not just the helpers.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
2026-08-21 14:44:59 -04:00

307 lines
13 KiB
Python

"""
Tests for src/web_interface/secret_helpers.py — the canonical secret
identification / separation / masking helpers.
This module is the extracted single source of truth for x-secret handling,
but until now had zero test coverage (only ``mask_secret_fields`` is even
imported by production code, from pages_v3). api_v3.py still carries three
inline re-implementations of ``find_secret_fields``/``separate_secrets`` —
see test_secret_separation_parity.py — so pinning the canonical behavior
here is a precondition for ever migrating those copies.
"""
import copy
from src.web_interface.secret_helpers import (
find_secret_fields,
separate_secrets,
mask_secret_fields,
mask_all_secret_values,
merge_secrets,
remove_empty_secrets,
)
SCHEMA_PROPS = {
"api_key": {"type": "string", "x-secret": True},
"city": {"type": "string"},
"auth": {
"type": "object",
"properties": {
"token": {"type": "string", "x-secret": True},
"username": {"type": "string"},
},
},
"accounts": {
"type": "array",
"items": {
"type": "object",
"properties": {
"name": {"type": "string"},
"token": {"type": "string", "x-secret": True},
},
},
},
"recovery_codes": {
"type": "array",
"items": {"type": "string", "x-secret": True},
},
}
class TestFindSecretFields:
def test_top_level_secret(self):
assert "api_key" in find_secret_fields(SCHEMA_PROPS)
def test_non_secret_not_included(self):
assert "city" not in find_secret_fields(SCHEMA_PROPS)
def test_nested_object_secret_uses_dot_path(self):
assert "auth.token" in find_secret_fields(SCHEMA_PROPS)
assert "auth.username" not in find_secret_fields(SCHEMA_PROPS)
def test_array_item_object_secret_uses_bracket_path(self):
assert "accounts[].token" in find_secret_fields(SCHEMA_PROPS)
def test_array_of_secrets_uses_bracket_path(self):
assert "recovery_codes[]" in find_secret_fields(SCHEMA_PROPS)
def test_full_set(self):
assert find_secret_fields(SCHEMA_PROPS) == {
"api_key", "auth.token", "accounts[].token", "recovery_codes[]",
}
def test_non_dict_properties_tolerated(self):
assert find_secret_fields({"weird": "not-a-dict"}) == set()
def test_non_dict_input_returns_empty(self):
assert find_secret_fields(None) == set()
assert find_secret_fields([]) == set()
class TestSeparateSecrets:
def test_flat_partition(self):
regular, secrets = separate_secrets(
{"api_key": "s3cret", "city": "Austin"}, {"api_key"})
assert regular == {"city": "Austin"}
assert secrets == {"api_key": "s3cret"}
def test_nested_partition(self):
config = {"auth": {"token": "t0k", "username": "chuck"}}
regular, secrets = separate_secrets(config, {"auth.token"})
assert regular == {"auth": {"username": "chuck"}}
assert secrets == {"auth": {"token": "t0k"}}
def test_empty_nested_dicts_pruned_from_regular(self):
# A dict that is all secrets leaves nothing behind on the regular
# side — the key must be dropped, not kept as {}.
config = {"auth": {"token": "t0k"}}
regular, secrets = separate_secrets(config, {"auth.token"})
assert regular == {}
assert secrets == {"auth": {"token": "t0k"}}
def test_whole_array_secret(self):
config = {"recovery_codes": ["a", "b"], "city": "Austin"}
regular, secrets = separate_secrets(config, {"recovery_codes[]"})
assert regular == {"city": "Austin"}
assert secrets == {"recovery_codes": ["a", "b"]}
def test_array_item_secrets_produce_parallel_lists(self):
# Per-item secrets keep the arrays index-aligned so they can be
# recombined: regular gets the stripped items, secrets a parallel
# list of the extracted values.
config = {"accounts": [
{"name": "a", "token": "ta"},
{"name": "b", "token": "tb"},
]}
regular, secrets = separate_secrets(config, {"accounts[].token"})
assert regular == {"accounts": [{"name": "a"}, {"name": "b"}]}
assert secrets == {"accounts": [{"token": "ta"}, {"token": "tb"}]}
def test_array_item_non_dict_items_get_placeholder(self):
config = {"accounts": [{"name": "a", "token": "ta"}, "oddball"]}
regular, secrets = separate_secrets(config, {"accounts[].token"})
assert regular == {"accounts": [{"name": "a"}, "oddball"]}
assert secrets == {"accounts": [{"token": "ta"}, {}]}
def test_array_without_secret_paths_stays_regular(self):
config = {"teams": ["DAL", "HOU"]}
regular, secrets = separate_secrets(config, {"api_key"})
assert regular == {"teams": ["DAL", "HOU"]}
assert secrets == {}
def test_round_trip_loses_nothing(self):
# separate + naive recombine must reconstruct the original config.
config = {
"api_key": "k",
"city": "Austin",
"auth": {"token": "t", "username": "chuck"},
"recovery_codes": ["a", "b"],
}
paths = find_secret_fields(SCHEMA_PROPS)
regular, secrets = separate_secrets(copy.deepcopy(config), paths)
def recombine(reg, sec):
out = copy.deepcopy(reg)
for k, v in sec.items():
if isinstance(v, dict) and isinstance(out.get(k), dict):
out[k] = recombine(out[k], v)
else:
out[k] = v
return out
assert recombine(regular, secrets) == config
class TestMaskSecretFields:
def test_masks_present_secret_to_empty_string(self):
result = mask_secret_fields({"api_key": "s3cret"}, SCHEMA_PROPS)
assert result["api_key"] == ""
def test_leaves_non_secret_untouched(self):
result = mask_secret_fields({"city": "Austin"}, SCHEMA_PROPS)
assert result["city"] == "Austin"
def test_none_and_empty_left_alone(self):
result = mask_secret_fields({"api_key": None}, SCHEMA_PROPS)
assert result["api_key"] is None
result = mask_secret_fields({"api_key": ""}, SCHEMA_PROPS)
assert result["api_key"] == ""
def test_falsey_but_set_values_are_masked(self):
# 0 and False are real values; the check is `is not None and != ''`.
# Note False == '' is False in Python, so False IS masked; 0 == '' is
# also False, so 0 is masked too.
result = mask_secret_fields({"api_key": 0}, SCHEMA_PROPS)
assert result["api_key"] == ""
result = mask_secret_fields({"api_key": False}, SCHEMA_PROPS)
assert result["api_key"] == ""
def test_nested_object_masked_without_mutating_input(self):
config = {"auth": {"token": "t0k", "username": "chuck"}}
original = copy.deepcopy(config)
result = mask_secret_fields(config, SCHEMA_PROPS)
assert result["auth"]["token"] == ""
assert result["auth"]["username"] == "chuck"
assert config == original # input not mutated
def test_array_of_secrets_masked_elementwise(self):
result = mask_secret_fields(
{"recovery_codes": ["a", "b"]}, SCHEMA_PROPS)
assert result["recovery_codes"] == ["", ""]
def test_array_of_objects_masked_per_item(self):
config = {"accounts": [{"name": "a", "token": "ta"}, "oddball"]}
result = mask_secret_fields(config, SCHEMA_PROPS)
assert result["accounts"][0] == {"name": "a", "token": ""}
assert result["accounts"][1] == "oddball"
def test_non_dict_schema_property_tolerated(self):
assert mask_secret_fields({"x": 1}, {"x": "bogus"}) == {"x": 1}
class TestMaskAllSecretValues:
def test_real_values_replaced_with_bullets(self):
assert mask_all_secret_values({"key": "abc"}) == {"key": "••••••••"}
def test_placeholders_preserved(self):
# YOUR_* placeholders must survive so the UI can show "not set".
result = mask_all_secret_values({"key": "YOUR_API_KEY_HERE"})
assert result == {"key": "YOUR_API_KEY_HERE"}
def test_empty_and_none_preserved(self):
assert mask_all_secret_values({"a": "", "b": None}) == {"a": "", "b": None}
def test_recurses_into_nested_dicts(self):
result = mask_all_secret_values({"plugin": {"token": "t", "empty": ""}})
assert result == {"plugin": {"token": "••••••••", "empty": ""}}
def test_non_string_real_values_masked(self):
assert mask_all_secret_values({"port": 8080}) == {"port": "••••••••"}
class TestRemoveEmptySecrets:
def test_strips_empty_string(self):
assert remove_empty_secrets({"a": "", "b": "real"}) == {"b": "real"}
def test_strips_whitespace_only(self):
assert remove_empty_secrets({"a": " "}) == {}
def test_strips_none(self):
assert remove_empty_secrets({"a": None}) == {}
def test_prunes_empty_nested_dicts(self):
assert remove_empty_secrets({"plugin": {"token": ""}}) == {}
def test_keeps_nested_real_values(self):
result = remove_empty_secrets({"plugin": {"token": "t", "empty": ""}})
assert result == {"plugin": {"token": "t"}}
def test_keeps_falsey_non_string_values(self):
# 0 and False are neither None nor blank strings — they are kept.
assert remove_empty_secrets({"a": 0, "b": False}) == {"a": 0, "b": False}
class TestArrayItemSecrets:
"""Lists merge by replacement, so a blanked array wipes stored credentials.
remove_empty_secrets recursed into dicts but let a list through untouched,
so [{"token": ""}] went straight into deep_merge and overwrote the stored
list. Saving any unrelated setting destroyed every token in the array.
"""
STORED = {"accounts": [{"name": "a", "token": "REAL-A"},
{"name": "b", "token": "REAL-B"}]}
def test_an_unrelated_save_keeps_every_stored_token(self):
posted = {"accounts": [{"name": "a", "token": ""},
{"name": "b", "token": ""}]}
merged = merge_secrets(self.STORED, remove_empty_secrets(posted))
assert [a["token"] for a in merged["accounts"]] == ["REAL-A", "REAL-B"]
def test_editing_one_entry_leaves_the_others_alone(self):
posted = {"accounts": [{"name": "a", "token": ""},
{"name": "b", "token": "NEW-B"}]}
merged = merge_secrets(self.STORED, remove_empty_secrets(posted))
assert [a["token"] for a in merged["accounts"]] == ["REAL-A", "NEW-B"]
def test_a_new_entry_is_appended(self):
posted = {"accounts": [{"name": "a", "token": ""},
{"name": "b", "token": ""},
{"name": "c", "token": "NEW-C"}]}
merged = merge_secrets(self.STORED, remove_empty_secrets(posted))
assert [a["token"] for a in merged["accounts"]] == \
["REAL-A", "REAL-B", "NEW-C"]
def test_a_list_of_bare_strings_merges_by_index(self):
merged = merge_secrets({"keys": ["K1", "K2", "K3"]},
remove_empty_secrets({"keys": ["", "K2-NEW", ""]}))
assert merged["keys"] == ["K1", "K2-NEW", "K3"]
def test_an_all_blank_list_is_dropped_entirely(self):
posted = {"accounts": [{"token": ""}, {"token": ""}]}
assert "accounts" not in remove_empty_secrets(posted)
def test_plain_dict_secrets_are_unaffected(self):
merged = merge_secrets({"api_key": "OLD", "other": "keep"},
remove_empty_secrets({"api_key": "", "other": "changed"}))
assert merged == {"api_key": "OLD", "other": "changed"}
def test_a_removed_entry_takes_its_secret_with_it(self):
"""The regular config's list is authoritative about how many items
exist, and the secrets list runs parallel to it -- see
ConfigManager._strip_secrets_recursive. So a shorter incoming list
must shorten the stored secrets too, or the two fall out of step."""
posted = {"accounts": [{"name": "a", "token": "NEW-A"}]}
merged = merge_secrets(self.STORED, remove_empty_secrets(posted))
assert [a["token"] for a in merged["accounts"]] == ["NEW-A"]
def test_an_emptied_item_stays_a_dict_not_none(self):
"""None there stops the list looking parallel, and
_strip_secrets_recursive then drops the whole key from the main
config -- deleting the item's non-secret fields as well."""
pruned = remove_empty_secrets(
{"accounts": [{"token": "real"}, {"token": ""}]})
assert pruned["accounts"] == [{"token": "real"}, {}]
assert None not in pruned["accounts"]