diff --git a/src/web_interface/secret_helpers.py b/src/web_interface/secret_helpers.py index 6a00cb94..9aa2eccc 100644 --- a/src/web_interface/secret_helpers.py +++ b/src/web_interface/secret_helpers.py @@ -5,7 +5,7 @@ Provides functions for identifying, masking, separating, and filtering secret fields in plugin configurations based on JSON Schema x-secret markers. """ -from typing import Any, Dict, Set, Tuple +from typing import Any, Dict, Optional, Set, Tuple def find_secret_fields(properties: Dict[str, Any], prefix: str = '') -> Set[str]: @@ -202,11 +202,89 @@ def remove_empty_secrets(secrets: Dict[str, Any]) -> Dict[str, Any]: nested = remove_empty_secrets(v) if nested: result[k] = nested + elif isinstance(v, list): + # Lists used to fall through to the scalar branch below and be + # kept verbatim, blanks and all. Because lists merge by + # *replacement*, saving any unrelated setting then wrote + # [{"token": ""}, ...] straight over the stored list and + # destroyed every credential in it. + pruned = _prune_secret_list(v) + if pruned is not None: + result[k] = pruned elif v is not None and not (isinstance(v, str) and v.strip() == ''): result[k] = v return result +def _prune_secret_list(items: list) -> Optional[list]: + """Strip blanks from inside a list of secrets, preserving every index. + + The rest of the system treats a secrets list as *parallel* to the regular + one -- ``sec[i]`` holds the secret fields of item ``i``, and ``{}`` means + "item i has none" (see ConfigManager._strip_secrets_recursive). So an + emptied dict item stays ``{}``: putting ``None`` there makes that list stop + looking parallel, and the stripper then drops the whole key from the main + config, taking the non-secret fields with it. + + A blank *scalar* becomes ``None``, meaning "no update at this index" -- + :func:`merge_secrets` substitutes whatever is stored there. Returns + ``None`` when nothing in the list carries a real value, so the caller drops + the key and leaves the stored list untouched. + """ + pruned: list = [] + has_real_value = False + for item in items: + if isinstance(item, dict): + kept = remove_empty_secrets(item) + pruned.append(kept) + has_real_value = has_real_value or bool(kept) + elif isinstance(item, list): + sub = _prune_secret_list(item) + pruned.append(sub if sub is not None else []) + has_real_value = has_real_value or sub is not None + elif item is not None and not (isinstance(item, str) and item.strip() == ''): + pruned.append(item) + has_real_value = True + else: + pruned.append(None) + return pruned if has_real_value else None + + +def merge_secrets(stored: Any, incoming: Any) -> Any: + """Merge submitted secrets over stored ones, element-wise inside lists. + + ``deep_merge`` replaces a list wholesale. For secrets that is destructive: + an incoming list that carries a real value for one entry and ``None`` for + the rest would drop the stored credentials of every other entry. Here a + list merges by index, and ``None`` means "keep what is stored". + + Entries are matched by *position*, which is what the config form gives us + -- there is no schema-declared identity to key on, and it is the same + contract ConfigManager._strip_secrets_recursive already relies on. The + incoming list's length wins, so deleting an item deletes its secrets; + an item the client left blank keeps whatever is stored at that index. + """ + if isinstance(stored, dict) and isinstance(incoming, dict): + merged = dict(stored) + for key, value in incoming.items(): + merged[key] = (merge_secrets(stored[key], value) + if key in stored else value) + return merged + if isinstance(stored, list) and isinstance(incoming, list): + # The incoming list sets the length -- the regular config's list is + # authoritative about how many items exist, and this one runs parallel + # to it. Removing an entry must therefore remove its secrets too. + merged_list = [] + for index, item in enumerate(incoming): + stored_item = stored[index] if index < len(stored) else None + merged_list.append(stored_item if item is None + else merge_secrets(stored_item, item)) + return merged_list + if incoming is None: + return stored + return incoming + + def strip_masked_values(secrets: Dict[str, Any]) -> Dict[str, Any]: """Remove values a client echoed back rather than changed. diff --git a/test/web_interface/test_api_v3_secret_roundtrip.py b/test/web_interface/test_api_v3_secret_roundtrip.py index 1d70f009..9d07a011 100644 --- a/test/web_interface/test_api_v3_secret_roundtrip.py +++ b/test/web_interface/test_api_v3_secret_roundtrip.py @@ -229,6 +229,44 @@ class TestSavePluginConfig: "REAL-KEY-0123456789", "an unrelated edit destroyed the API key" assert env.fresh_load()[PLUGIN_ID]["city"] == "Dallas" + def test_an_unrelated_edit_does_not_erase_array_item_secrets(self, env): + """The scalar api_key case above, but for a list of credentials. + + remove_empty_secrets recursed into dicts only, so a list went into + deep_merge untouched -- and lists merge by *replacement*. Saving any + unrelated field posted [{"token": ""}, ...] straight over the stored + array and destroyed every token in it at once. + """ + assert self._save(env, {"accounts": [ + {"name": "a", "token": "REAL-A"}, + {"name": "b", "token": "REAL-B"}, + ], "city": "Austin"}).status_code == 200 + + # the user changes the city; both masked tokens ride along blank + assert self._save(env, {"accounts": [ + {"name": "a", "token": ""}, + {"name": "b", "token": ""}, + ], "city": "Dallas"}).status_code == 200 + + merged = env.fresh_load()[PLUGIN_ID] + assert [a.get("token") for a in merged["accounts"]] == \ + ["REAL-A", "REAL-B"], "an unrelated edit destroyed the array secrets" + assert [a["name"] for a in merged["accounts"]] == ["a", "b"] + assert merged["city"] == "Dallas" + + def test_one_array_secret_can_be_changed_without_losing_the_rest(self, env): + assert self._save(env, {"accounts": [ + {"name": "a", "token": "REAL-A"}, + {"name": "b", "token": "REAL-B"}, + ]}).status_code == 200 + assert self._save(env, {"accounts": [ + {"name": "a", "token": ""}, + {"name": "b", "token": "NEW-B"}, + ]}).status_code == 200 + + merged = env.fresh_load()[PLUGIN_ID] + assert [a.get("token") for a in merged["accounts"]] == ["REAL-A", "NEW-B"] + def test_a_secret_can_still_be_changed(self, env): """Dropping blanks must not stop a real new value from being saved.""" self._save(env, {"api_key": "first-key"}) diff --git a/test/web_interface/test_config_logging_omits_secrets.py b/test/web_interface/test_config_logging_omits_secrets.py new file mode 100644 index 00000000..031eb4f1 --- /dev/null +++ b/test/web_interface/test_config_logging_omits_secrets.py @@ -0,0 +1,45 @@ +"""The validation logging ran before separate_secrets, so it logged credentials. + +api_v3's plugin-config save 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 the user just typed +into the form -- API keys and tokens went to the journal in clear text. +""" +import re +from pathlib import Path + +import pytest + +SOURCE = (Path(__file__).resolve().parents[2] + / "web_interface" / "blueprints" / "api_v3.py") + +#: Objects that still hold submitted secret values at the point these log +#: calls run. Interpolating one whole into a log message leaks credentials. +UNREDACTED = ("plugin_config", "secrets_config", "current_secrets") + + +def _logging_lines(): + for number, line in enumerate(SOURCE.read_text(encoding="utf-8").splitlines(), 1): + stripped = line.strip() + if stripped.startswith("#"): + continue + if re.match(r"logger\.(debug|info|warning|error|critical|exception)\(", stripped): + yield number, stripped + + +@pytest.mark.parametrize("name", UNREDACTED) +def test_no_log_call_interpolates_a_whole_secret_bearing_object(name): + # {name} or {name['k']} leaks; {list(name.keys())} and {len(name)} do not. + bare = re.compile(r"\{" + re.escape(name) + r"(\[[^\]]*\])*\}") + offenders = [f"{n}: {text}" for n, text in _logging_lines() if bare.search(text)] + assert not offenders, ( + f"{name} still holds submitted secrets where these log calls run:\n " + + "\n ".join(offenders)) + + +def test_the_guard_would_notice_a_reintroduced_leak(): + """Pin the detector itself, so a rewrite cannot silently stop matching.""" + bare = re.compile(r"\{" + re.escape("plugin_config") + r"(\[[^\]]*\])*\}") + assert bare.search('logger.info(f"Full config: {plugin_config}")') + assert bare.search("logger.error(f\"{plugin_config['api_key']}\")") + assert not bare.search('logger.info(f"{list(plugin_config.keys())}")') diff --git a/test/web_interface/test_secret_helpers.py b/test/web_interface/test_secret_helpers.py index 5f33e899..a9eb4a7a 100644 --- a/test/web_interface/test_secret_helpers.py +++ b/test/web_interface/test_secret_helpers.py @@ -17,6 +17,7 @@ from src.web_interface.secret_helpers import ( separate_secrets, mask_secret_fields, mask_all_secret_values, + merge_secrets, remove_empty_secrets, ) @@ -239,3 +240,67 @@ class TestRemoveEmptySecrets: 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"] diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 37df1141..958f5f0c 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -22,7 +22,8 @@ logger = logging.getLogger(__name__) from src.web_interface.api_helpers import success_response, error_response, validate_request_json from src.web_interface.errors import ErrorCode from src.web_interface.secret_helpers import (find_secret_fields, mask_all_secret_values, - remove_empty_secrets, separate_secrets, + merge_secrets, remove_empty_secrets, + separate_secrets, strip_masked_values) from src.web_interface.error_handler import describe_exception, redact_text from src.plugin_system.operation_types import OperationType @@ -1296,7 +1297,10 @@ def save_main_config(): if secrets_config: if plugin_id not in current_secrets: current_secrets[plugin_id] = {} - current_secrets[plugin_id] = deep_merge(current_secrets[plugin_id], secrets_config) + # Lists merge by replacement, so deep_merge here wrote a + # blanked array straight over the stored credentials. + current_secrets[plugin_id] = merge_secrets( + current_secrets[plugin_id], secrets_config) # Save secrets file api_v3.config_manager.save_raw_file_content('secrets', current_secrets) @@ -5675,8 +5679,10 @@ def save_plugin_config(): if schema: # Log what we're validating for debugging logger.info(f"Validating config for {plugin_id}") + # Only the shape. plugin_config still holds the submitted secret + # values at this point -- separate_secrets does not run until + # below -- so logging it wrote live credentials to the journal. logger.info(f"Config keys being validated: {list(plugin_config.keys())}") - logger.info(f"Full config: {plugin_config}") # Get enhanced schema keys (including injected core properties) # We need to create an enhanced schema to get the actual allowed keys @@ -5699,7 +5705,8 @@ def save_plugin_config(): # Log validation errors for debugging logger.error(f"Config validation failed for {plugin_id}") logger.error(f"Validation errors: {validation_errors}") - logger.error(f"Config that failed: {plugin_config}") + # Keys only, for the same reason as above. + logger.error(f"Config keys that failed: {list(plugin_config.keys())}") logger.error(f"Schema properties: {list(enhanced_schema.get('properties', {}).keys())}") # Also print to console for immediate visibility @@ -5750,7 +5757,9 @@ def save_plugin_config(): if secrets_config: if plugin_id not in current_secrets: current_secrets[plugin_id] = {} - current_secrets[plugin_id] = deep_merge(current_secrets[plugin_id], secrets_config) + # See above -- secrets lists must merge element-wise. + current_secrets[plugin_id] = merge_secrets( + current_secrets[plugin_id], secrets_config) # Save secrets file try: api_v3.config_manager.save_raw_file_content('secrets', current_secrets) diff --git a/web_interface/static/v3/app.js b/web_interface/static/v3/app.js index 581c66f2..638204b3 100644 --- a/web_interface/static/v3/app.js +++ b/web_interface/static/v3/app.js @@ -126,7 +126,17 @@ window.showRestartPending = function(message) { } catch { /* private browsing */ } const banner = document.getElementById('restart-pending-banner'); const text = document.getElementById('restart-pending-text'); - if (text && message) text.textContent = message; + if (text) { + // Without the else-branch a config save inherited whatever wording the + // previous update left in the DOM: showRestartPending() clears the + // stored text but used to leave the element itself alone. The default + // is read back from the server-rendered copy rather than duplicated + // here, so the template stays the one place that owns the string. + if (text.dataset.defaultText === undefined) { + text.dataset.defaultText = text.textContent.trim(); + } + text.textContent = message || text.dataset.defaultText; + } if (banner) banner.style.display = 'block'; };