refactor(web): use canonical secret helpers in api_v3; make ConfigManager secret strip/merge array-aware

api_v3.py carried three inline nested copies of find_secret_fields/
separate_secrets (main-config save, plugin-config save, plugin-config
reset). They drifted from each other (one lacked isinstance guards) and
none supported the canonical module's array-item secrets
(accounts[].token). All three endpoints now import from
src/web_interface/secret_helpers.

Adopting the canonical behavior makes array-item secrets reachable, and
their parallel-placeholder shape ([{'token': ...}, {}] alongside the
regular list) was not survivable by ConfigManager's round-trip:
_strip_secrets_recursive dropped the whole key (losing the regular
fields from config.json) and _deep_merge replaced the regular list
wholesale on load. Both are now array-aware:

- strip removes the secret fields from each item and ALWAYS keeps the
  list so indices survive for merge-on-load; whole-key secrets (scalar
  lists, shape mismatches) still drop the key entirely — never leak.
- merge folds each secrets item into the config item at the same index,
  skipping {} placeholders. The regular list's length is authoritative
  in both directions: a user deleting an array item never has it
  resurrected from a stale secrets entry (extras warn and are ignored).

api_v3's own deep_merge intentionally still replaces lists wholesale —
form posts carry complete arrays and index-merging would resurrect
deleted items; a comment now documents that.

Tests: the parity guard flips from 'exactly 3 inline copies' to 'zero,
and the canonical import must exist'; TestArraySecretStripAndMerge
covers the new strip/merge semantics incl. length-mismatch contracts;
new test_api_v3_secret_roundtrip.py drives all three endpoints through
a Flask client with a REAL ConfigManager+SchemaManager over tmp_path,
proving secrets land in config_secrets.json, config.json stays clean,
and a fresh load merges them back into the right array items.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NohXi78cwsAKtN1sCfxjUh
This commit is contained in:
Claude
2026-08-07 17:33:47 +00:00
parent 003312f4ff
commit 2efd49a1ab
5 changed files with 478 additions and 173 deletions
+122
View File
@@ -177,3 +177,125 @@ class TestLoadFastPath:
os.utime(config_file, ns=(1_000_000_000, 1_000_000_000))
assert manager.load_config()["timezone"] == "AAA" # stale, by design
class TestArraySecretStripAndMerge:
"""Array-item secrets round-trip (parallel-placeholder lists).
secret_helpers.separate_secrets emits array secrets as a list parallel
to the regular list, with {} for items that carry no secrets. Strip
must remove the secret fields from config.json while preserving item
indices; load must merge them back into the right items. The regular
list's length is authoritative in both directions.
"""
def test_strip_removes_array_item_secrets_keeps_indices(self, tmp_path):
manager = make_manager(tmp_path)
data = {"plugin": {"accounts": [
{"name": "a", "token": "ta"},
{"name": "b"},
]}}
secrets = {"plugin": {"accounts": [{"token": "ta"}, {}]}}
stripped = manager._strip_secrets_recursive(data, secrets)
assert stripped == {"plugin": {"accounts": [{"name": "a"}, {"name": "b"}]}}
def test_strip_keeps_all_placeholder_items(self, tmp_path):
# Even when every item strips to nothing extra, the list survives
# with its indices — required for merge-on-load alignment.
manager = make_manager(tmp_path)
data = {"accounts": [{"token": "t1"}, {"token": "t2"}]}
secrets = {"accounts": [{"token": "t1"}, {"token": "t2"}]}
stripped = manager._strip_secrets_recursive(data, secrets)
assert stripped == {"accounts": [{}, {}]}
def test_strip_whole_scalar_array_secret_drops_key(self, tmp_path):
# A list of secret scalars is a whole-key secret, not the parallel
# shape — the key must vanish from config.json entirely.
manager = make_manager(tmp_path)
data = {"recovery_codes": ["a", "b"], "city": "Austin"}
secrets = {"recovery_codes": ["a", "b"]}
stripped = manager._strip_secrets_recursive(data, secrets)
assert stripped == {"city": "Austin"}
def test_strip_shape_mismatch_drops_key(self, tmp_path):
# Conservative contract: if the shapes disagree, never leak.
manager = make_manager(tmp_path)
data = {"accounts": {"name": "not-a-list"}}
secrets = {"accounts": [{"token": "t"}]}
stripped = manager._strip_secrets_recursive(data, secrets)
assert stripped == {}
def test_strip_ignores_extra_secrets_entries(self, tmp_path):
# Regular list length is authoritative: a user deleted an item.
manager = make_manager(tmp_path)
data = {"accounts": [{"name": "a", "token": "ta"}]}
secrets = {"accounts": [{"token": "ta"}, {"token": "tb"}]}
stripped = manager._strip_secrets_recursive(data, secrets)
assert stripped == {"accounts": [{"name": "a"}]}
def test_merge_restores_array_item_secrets(self, tmp_path):
manager = make_manager(tmp_path)
target = {"accounts": [{"name": "a"}, {"name": "b"}]}
manager._deep_merge(target, {"accounts": [{"token": "ta"}, {}]})
assert target == {"accounts": [
{"name": "a", "token": "ta"},
{"name": "b"},
]}
def test_merge_ignores_extra_secrets_entries_with_warning(self, tmp_path, caplog):
manager = make_manager(tmp_path)
target = {"accounts": [{"name": "a"}]}
with caplog.at_level("WARNING"):
manager._deep_merge(
target, {"accounts": [{"token": "ta"}, {"token": "ghost"}]})
assert target == {"accounts": [{"name": "a", "token": "ta"}]}
assert any("longer than the config list" in r.message for r in caplog.records)
def test_merge_non_dict_item_replaced_by_secret(self, tmp_path):
# Shape drift inside the list: the secret wins for that index.
manager = make_manager(tmp_path)
target = {"accounts": ["oddball", {"name": "b"}]}
manager._deep_merge(target, {"accounts": [{"token": "ta"}, {}]})
assert target == {"accounts": [{"token": "ta"}, {"name": "b"}]}
def test_merge_whole_scalar_array_still_replaces(self, tmp_path):
# Legacy behavior preserved: a non-parallel list replaces wholesale.
manager = make_manager(tmp_path)
target = {"recovery_codes": ["old"]}
manager._deep_merge(target, {"recovery_codes": ["new1", "new2"]})
assert target == {"recovery_codes": ["new1", "new2"]}
def test_full_save_load_round_trip(self, tmp_path):
# End to end on real files: save strips array secrets out of
# config.json; load merges them back into the right items.
manager = make_manager(
tmp_path,
config={"plugin": {"accounts": [
{"name": "a", "token": "s3cret-a"},
{"name": "b", "token": "s3cret-b"},
]}},
secrets={"plugin": {"accounts": [
{"token": "s3cret-a"}, {"token": "s3cret-b"},
]}},
)
loaded = manager.load_config()
assert loaded["plugin"]["accounts"][0]["token"] == "s3cret-a"
manager.save_config(loaded)
raw = (tmp_path / "config.json").read_text()
assert "s3cret" not in raw
on_disk = json.loads(raw)
assert on_disk["plugin"]["accounts"] == [{"name": "a"}, {"name": "b"}]
# A fresh manager (constructed directly — make_manager would
# overwrite the just-saved config.json) re-merges from the secrets
# file on load.
fresh = ConfigManager(config_path=str(tmp_path / "config.json"),
secrets_path=str(tmp_path / "config_secrets.json"))
fresh.template_path = str(tmp_path / "no-template.json")
reloaded = fresh.load_config()
assert reloaded["plugin"]["accounts"] == [
{"name": "a", "token": "s3cret-a"},
{"name": "b", "token": "s3cret-b"},
]