From 95bd8a1a67a721db1b51772bfbf61392708d2d6e Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Thu, 20 Aug 2026 05:53:51 -0400 Subject: [PATCH] fix(web): stop an unrelated config edit from erasing a plugin's secret Saving any field on a plugin's config form destroyed that plugin's stored credential. On a rig with a weather API key, changing the city silently emptied the key, and the plugin stopped working at the next fetch with no indication why. The path had no guard at any step. The config partial masks secrets before rendering (pages_v3.py:740), so the browser posts them back blank; _parse_value deliberately preserves "" for optional string fields; separate_secrets routes that "" into secrets_config, which is a truthy dict; deep_merge writes it over the stored value; save_raw_file_content persists it. The blank does not even need the round-trip. merge_with_defaults injects the schema's api_key default ("") into every save, so a client that never sends the field at all still erases it. test_secret_count_message_counts_top_level_keys was counting exactly that injected blank as a saved secret field -- the visible edge of the bug, pinned as expected behaviour. remove_empty_secrets() already existed for this, with seven unit tests and a docstring describing this precise scenario ("clients will send those empty strings back ... so that existing stored secrets are not overwritten with blanks"). It was never wired into a call site. This wires it into both save paths that merge into the secrets file. A blank now means "unchanged" rather than "delete", which is the same contract the helper's tests already describe. The cost is that a secret can no longer be cleared by emptying the field; clearing needs its own affordance, since a control that erases credentials as a side effect of ordinary edits is not one. Verified by reverting the guard: the new round-trip test then fails with the stored key read back as ''. 262 web tests pass with it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --- .../test_api_v3_secret_roundtrip.py | 41 ++++++++++++++++--- web_interface/blueprints/api_v3.py | 13 +++++- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/test/web_interface/test_api_v3_secret_roundtrip.py b/test/web_interface/test_api_v3_secret_roundtrip.py index a4648824..1d70f009 100644 --- a/test/web_interface/test_api_v3_secret_roundtrip.py +++ b/test/web_interface/test_api_v3_secret_roundtrip.py @@ -194,15 +194,46 @@ class TestSavePluginConfig: def test_secret_count_message_counts_top_level_keys(self, env): # Pinned: the "(N secret field(s))" message counts TOP-LEVEL keys of - # the separated secrets dict. Here that is 2: the posted accounts - # array (all its item tokens count as ONE key) plus the schema's - # api_key default ("") that merge_with_defaults adds before - # separation. + # the separated secrets dict. Here that is 1: the posted accounts + # array, whose item tokens all count as ONE key. + # + # It was 2 before blank secrets were dropped, the second being the + # schema's api_key default (""), which merge_with_defaults adds to + # every save. Counting it was the visible edge of a real bug: that + # injected blank was merged over the stored api_key, so saving any + # unrelated field destroyed the credential. See + # test_an_unrelated_edit_does_not_erase_a_stored_secret. resp = self._save(env, { "accounts": [{"name": "a", "token": "t"}], }) message = resp.get_json()["message"] - assert "(2 secret field(s) saved to config_secrets.json)" in message + assert "(1 secret field(s) saved to config_secrets.json)" in message + + def test_an_unrelated_edit_does_not_erase_a_stored_secret(self, env): + """Editing one field must not wipe the plugin's API key. + + The config form renders secrets masked, so the browser posts them + back blank; merge_with_defaults injects a blank api_key even when + the client omits it entirely. Either way a "" reached the secrets + file and deep_merge wrote it over the stored credential. + """ + assert self._save(env, {"api_key": "REAL-KEY-0123456789", + "city": "Austin"}).status_code == 200 + assert _on_disk(env.secrets_file)[PLUGIN_ID]["api_key"] == \ + "REAL-KEY-0123456789" + + # the user changes the city; the masked api_key rides along blank + assert self._save(env, {"api_key": "", "city": "Dallas"}).status_code == 200 + + assert _on_disk(env.secrets_file)[PLUGIN_ID]["api_key"] == \ + "REAL-KEY-0123456789", "an unrelated edit destroyed the API key" + assert env.fresh_load()[PLUGIN_ID]["city"] == "Dallas" + + 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"}) + self._save(env, {"api_key": "second-key"}) + assert _on_disk(env.secrets_file)[PLUGIN_ID]["api_key"] == "second-key" def test_resave_replaces_stored_secrets_list_wholesale(self, env): # Characterized: api_v3's deep_merge intentionally replaces lists, diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 984637e8..c6f7c829 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -21,7 +21,8 @@ logger = logging.getLogger(__name__) # Import new infrastructure 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, separate_secrets +from src.web_interface.secret_helpers import (find_secret_fields, remove_empty_secrets, + separate_secrets) from src.web_interface.error_handler import describe_exception, redact_text from src.plugin_system.operation_types import OperationType from src.web_interface.validators import ( @@ -1216,6 +1217,11 @@ def save_main_config(): # Separate secrets from regular config (same logic as save_plugin_config) regular_config, secrets_config = separate_secrets(plugin_config, secret_fields) + # The config form renders secrets masked, so every save posts + # them back blank. Without this the blank is merged over the + # stored value and the credential is destroyed by the act of + # changing an unrelated setting. A blank means "unchanged". + secrets_config = remove_empty_secrets(secrets_config) # PRE-PROCESSING: Preserve 'enabled' state if not in regular_config # This prevents overwriting the enabled state when saving config from a form that doesn't include the toggle @@ -5599,6 +5605,11 @@ def save_plugin_config(): # Separate secrets from regular config (handles nested configs and # array-item secrets — see src/web_interface/secret_helpers.py) regular_config, secrets_config = separate_secrets(plugin_config, secret_fields) + # The config form renders secrets masked, so every save posts + # them back blank. Without this the blank is merged over the + # stored value and the credential is destroyed by the act of + # changing an unrelated setting. A blank means "unchanged". + secrets_config = remove_empty_secrets(secrets_config) # Get current configs current_config = api_v3.config_manager.load_config()