fix(web): plugin settings endpoints - a refused save no longer leaks into config.json; GET masks secrets

CHANGELOG.md

@@ -532,6 +532,57 @@ policies are unchanged.
   the plugin leaves rotation until the cooldown ends, the same as a raising
   `update()`. The display still moves straight on to the next mode. A hung
   `display()` is still recorded once, as a hang.
+- A plugin settings save that failed validation no longer leaks into the next
+  save. `ConfigManager.load_config()` returned its cached config itself (the
+  fast path from #410), so the form save's edits went into the cache before
+  validation ran, and a refused save left them there. The next save of any
+  other setting (another plugin's, a plugin toggle, the schedule) wrote them
+  to config.json: the refused value, and a nested secret typed into the same
+  form (`mqtt.password`, `league.espn_s2`, `flightaware.api_key`) in plain
+  text, because it had never reached config_secrets.json to be stripped.
+  The form also reloaded showing the refused values. `load_config()` now
+  returns a private copy, and the saves keep one, so nothing a caller edits
+  reaches the cache unless it is saved. The copy duplicates only the dicts
+  and lists (every other JSON value is immutable): 2.1 ms for a real 60 KiB
+  config on a Pi 4, against 6.8 ms for `copy.deepcopy`.
+- `GET /api/v3/plugins/config` no longer returns secrets. It sent back the
+  plugin's section with config_secrets.json merged in, API keys and tokens
+  in plain text: the masking #276 added was dropped in #330. It also took
+  any id, so `?plugin_id=web_auth` returned the login's cookie-signing key
+  and password hash and `?plugin_id=github` the Plugin Store token. Secret
+  fields now come back blank, as the settings page renders them, and a
+  plugin with no schema has its credential-named fields blanked, as
+  `GET /config/main` does. Blank rather than the `••••••••` of
+  `GET /config/secrets`, because the save reads a blank secret as
+  "unchanged", so a client can post the response back without erasing
+  one. Core sections and malformed ids get a 400, as they already did from
+  reset and uninstall.
+- Plugin settings with a table (a list of rows, such as geochron's cities
+  or the countdowns) save again when a text cell is blank or holds only
+  digits. A row posts its cells as `cities.0.timezone`, and the schema
+  lookup stopped at the list, so each cell was parsed with no schema: a
+  blank optional text cell became null, and a name like "2027" became a
+  number. Either failed validation, and every save of the page failed for
+  as long as the row existed. A plugin with a secret in its rows could not
+  be saved from the page at all, since the secret cell is drawn blank. The
+  lookup now steps from the index into the list's item schema.
+- A plugin whose API key is required and has no default (youtube-stats)
+  can be saved from its settings page without typing the key in again. The
+  page draws a stored secret blank and posts the blank back; for a required
+  secret the save read that blank as null, failed validation, and refused
+  every save of the page. A blank secret field now means "unchanged", as it
+  already did for an optional one.
+- `POST /api/v3/plugins/config` refuses a core section or a malformed
+  plugin id with a 400, as reset and uninstall already did.
+  `{"plugin_id": "display", ...}` merged unvalidated values into the core
+  display section (and added `"enabled": true` to it), and an id that was
+  not a string answered with a 500.
+- A plugin text setting saves what was typed when that looks like a
+  boolean or JSON. The form save tried `true`/`false` and `[...]`/`{...}`
+  before it looked at the schema, so a text field holding "true", "False",
+  "[1, 2]" or "{}" was stored as a boolean, list or object, and the save
+  failed validation. Text fields, nullable ones included, are now taken as
+  typed; other types convert as before.
 - A WiFi notice (such as "Connected to HomeNet" or "AP mode on") now shows
   within about a second of being posted. It was only checked between
   screens, so a 5 s notice posted during a 20 s screen expired before that

src/config_manager.py

@@ -46,6 +46,35 @@
     get_config_dir_mode
 )
 
+
+def _private_copy(config: Dict[str, Any]) -> Dict[str, Any]:
+    """A deep copy of ``config`` that shares nothing with it.
+
+    load_config() hands one out per call, and the saves keep one, so the
+    cached config is never an object a caller holds. A web handler edits what
+    it loaded, validates, and may refuse the save; when the cache was that
+    same object, the refused edit stayed in it, and the next save of any
+    other setting wrote it to config.json -- a nested secret included, in
+    plain text, since it had never reached config_secrets.json to be
+    stripped.
+
+    The config is JSON data, so only its dicts and lists need copying; every
+    other value in it is immutable. On a Pi 4 with a real 60 KiB config this
+    takes 2.1 ms against copy.deepcopy's 6.8 ms, on a path ~30 handlers call
+    (a pickle round trip is no faster, 1.9 ms, and brings pickle into the
+    config path for nothing).
+    """
+    return _copy_containers(config)
+
+
+def _copy_containers(value: Any) -> Any:
+    if isinstance(value, dict):
+        return {key: _copy_containers(item) for key, item in value.items()}
+    if isinstance(value, list):
+        return [_copy_containers(item) for item in value]
+    return value
+
+
 class ConfigManager:
     """
     Reads and writes the main application configuration files.
@@ -126,9 +155,10 @@ def save_config_atomic(
             validate_after_write=validate_after_write
         )
 
-        # Update in-memory config if save was successful
+        # Update in-memory config if save was successful. A copy: the caller
+        # still holds new_config_data (see _private_copy).
         if result.status == SaveResultStatus.SUCCESS:
-            self.config = new_config_data
+            self.config = _private_copy(new_config_data)
             # In-memory config now matches what was just written, so the
             # load_config fast path may return it. It still carries the
             # merged secrets that were stripped on disk; that matches a full
@@ -208,14 +238,16 @@ def load_config(self) -> Dict[str, Any]:
 
         Fast path: when config.json, config_secrets.json and the template
         are all unchanged since the last successful load (mtime_ns + size),
-        the already-parsed self.config is returned without touching the
-        files — same aliasing semantics as the full path, which also
-        returns self.config.
+        a copy of the already-parsed self.config is returned without
+        touching the files.
+
+        Either way the caller gets its own copy (see _private_copy): editing
+        it changes nothing here until it is saved.
         """
         try:
             current_sig = self._files_signature()
             if self.config and self._loaded_sig == current_sig:
-                return self.config
+                return _private_copy(self.config)
 
             # Check if config file exists, if not create from template
             if not os.path.exists(self.config_path):
@@ -249,8 +281,8 @@ def load_config(self) -> Dict[str, Any]:
             # Signature taken AFTER load + migration (migration may write the
             # config back), so it reflects exactly what was read/written.
             self._loaded_sig = self._files_signature()
-            return self.config
-            
+            return _private_copy(self.config)
+
         except FileNotFoundError as e:
             # Only config.json can get here: a missing or unreadable secrets
             # file is handled where it is read.
@@ -355,8 +387,9 @@ def save_config(self, new_config_data: Dict[str, Any]) -> None:
         try:
             atomic_write_json(self.config_path, config_to_write)
 
-            # Update the in-memory config to the new state (which includes secrets for runtime)
-            self.config = new_config_data
+            # Update the in-memory config to the new state (which includes
+            # secrets for runtime), as a copy -- see _private_copy
+            self.config = _private_copy(new_config_data)
             self._loaded_sig = self._files_signature()
             self.logger.info(f"Configuration successfully saved to {os.path.abspath(self.config_path)}")
             if secrets_content:

test/test_config_load_cache.py

@@ -58,7 +58,8 @@ def test_unchanged_files_are_not_reread(self, mgr, monkeypatch):
         for _ in range(10):
             again = m.load_config()
         assert counts["n"] == 0, "fast path must not re-open any config file"
-        assert again is first  # same aliasing semantics as the full path
+        assert again == first
+        assert again is not first  # each caller gets its own copy, see below
 
     def test_config_change_triggers_reload(self, mgr):
         m, config, secrets, template = mgr
@@ -98,6 +99,33 @@ def test_same_second_edit_detected_via_mtime_ns_or_size(self, mgr):
         assert m.load_config()["timezone"] == "America/New_York"
 
 
+class TestCallersGetACopy:
+    """A web handler edits what load_config returned, then validates. When
+    validation failed, the edit stayed in the cache the fast path serves, and
+    the next unrelated save wrote it -- a nested secret included, in plain
+    text, because it had never reached config_secrets.json to be stripped."""
+
+    def test_editing_a_loaded_config_does_not_change_the_next_load(self, mgr):
+        m, config, secrets, template = mgr
+        loaded = m.load_config()
+        loaded["display"]["brightness"] = 1
+        loaded["weather"]["api_key"] = "typed-but-never-saved"
+        again = m.load_config()
+        assert again["display"]["brightness"] == 90
+        assert again["weather"]["api_key"] == "sek"
+
+    def test_the_full_path_also_returns_a_copy(self, mgr):
+        m, config, secrets, template = mgr
+        m.load_config()["display"]["brightness"] = 1  # first load: full path
+        assert m.load_config()["display"]["brightness"] == 90
+
+    def test_an_edit_never_reaches_a_later_save(self, mgr):
+        m, config, secrets, template = mgr
+        m.load_config()["display"]["new_secret"] = "hunter2"  # then bailed out
+        m.save_config(m.load_config())  # some other handler saves
+        assert "hunter2" not in config.read_text()
+
+
 class TestSaveCoherence:
     def test_save_config_then_load_returns_saved_data(self, mgr, monkeypatch):
         m, config, secrets, template = mgr
@@ -111,6 +139,15 @@ def test_save_config_then_load_returns_saved_data(self, mgr, monkeypatch):
         assert loaded["weather"]["api_key"] == "sek"  # secrets survive in memory
         assert counts["n"] == 0  # signature refreshed by save; no re-read
 
+    def test_the_saved_dict_does_not_become_the_cache(self, mgr):
+        m, config, secrets, template = mgr
+        m.load_config()
+        new = {"display": {"brightness": 42}, "timezone": "UTC",
+               "weather": {"api_key": "sek"}}
+        m.save_config(new)
+        new["display"]["brightness"] = 7  # the caller keeps using its dict
+        assert m.load_config()["display"]["brightness"] == 42
+
     def test_cross_process_save_is_picked_up(self, mgr):
         """Another process writing config.json (different mtime) must bust
         this process's fast path — the core cross-process guarantee."""

test/test_config_manager_secrets.py

@@ -143,7 +143,10 @@ def test_unchanged_files_return_cached_dict(self, tmp_path):
         manager = make_manager(tmp_path, config={"timezone": "UTC"})
         first = manager.load_config()
         second = manager.load_config()
-        assert second is first  # same aliased dict, no re-read
+        # A copy of the cached dict, never the dict itself; that it is not
+        # re-read is test_config_load_cache's test_unchanged_files_are_not_reread
+        assert second == first
+        assert second is not first
 
     def test_touching_secrets_file_invalidates_cache(self, tmp_path):
         manager = make_manager(

test/web_interface/test_api_v3_helpers.py

@@ -169,6 +169,10 @@ class TestGetSchemaProperty:
             },
             "fifa.world": {"type": "object",
                            "properties": {"enabled": {"type": "boolean"}}},
+            "cities": {"type": "array",
+                       "items": {"type": "object",
+                                 "properties": {"timezone": {"type": "string"}}}},
+            "color": {"type": ["array", "null"], "items": {"type": "integer"}},
         }
     }
 
@@ -185,6 +189,15 @@ def test_dotted_schema_key_matched_longest_first(self):
         prop = _get_schema_property(self.SCHEMA, "fifa.world.enabled")
         assert prop == {"type": "boolean"}
 
+    def test_an_index_steps_into_the_array_items(self):
+        # How a table row posts its cells
+        assert _get_schema_property(self.SCHEMA, "cities.0.timezone") == {"type": "string"}
+        assert _get_schema_property(self.SCHEMA, "color.2") == {"type": "integer"}
+
+    def test_a_non_index_under_an_array_is_not_found(self):
+        assert _get_schema_property(self.SCHEMA, "cities.timezone") is None
+        assert _get_schema_property(self.SCHEMA, "cities.0.nope") is None
+
     def test_missing_path_returns_none(self):
         assert _get_schema_property(self.SCHEMA, "nope.nope") is None