From 46afbaf81edc69c61d29d2d588aa9705ac50460e Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 20:57:12 -0400 Subject: [PATCH 1/8] fix(config): load_config hands each caller a private copy ConfigManager.load_config() returned its cached self.config itself (the mtime fast path from #410 kept the full path's aliasing). Web handlers edit what they load and then validate: the plugin form save applies the posted fields to the loaded section (a shallow .copy(), so nested dicts were the cache's own), and save_main_config sets its checkboxes before it checks auto_update_channel. When the save was refused, the edit stayed in the cache the fast path serves, and the next save of any other setting wrote it 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, since it never reached config_secrets.json to be stripped. The form also reloaded showing the refused values. load_config() now returns a private copy on both paths, and save_config/save_config_atomic keep a copy of what they were given, so nothing a caller edits reaches the cache unless it is saved. Fixing it here rather than in each handler covers every route that edits before it validates. No caller relies on editing the cache without saving: every src/ and web_interface/ caller either reads, or saves the dict it edited. get_config() still returns the live dict for the display process's readers. The copy is a pickle round trip: on a Pi 4 with its real 64 KiB config, 2.0 ms against 6.9 ms for copy.deepcopy (json round trip 3.4 ms). Two tests asserted the aliasing itself and now assert a copy. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 ++ src/config_manager.py | 44 ++++- test/test_config_load_cache.py | 39 +++- test/test_config_manager_secrets.py | 5 +- .../test_plugin_config_endpoints.py | 172 ++++++++++++++++++ 5 files changed, 261 insertions(+), 12 deletions(-) create mode 100644 test/web_interface/test_plugin_config_endpoints.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8fa3a3862..cd0ea5553 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -532,6 +532,19 @@ 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 is a pickle round trip: + 2.0 ms for a real 64 KiB config on a Pi 4, against 6.9 ms for + `copy.deepcopy`. - 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 diff --git a/src/config_manager.py b/src/config_manager.py index dd7982a36..cd70bd0fb 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -31,6 +31,7 @@ import json import os import logging +import pickle from pathlib import Path from typing import Dict, Any, Optional, List from src.core_config_keys import CORE_CONFIG_KEYS, CORE_SECRETS_KEYS @@ -46,6 +47,25 @@ 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. + + A pickle round trip rather than copy.deepcopy: the config is plain JSON + data, and on a Pi 4 with a real 64 KiB config this takes 2.0 ms against + deepcopy's 6.9 ms, on a path ~30 handlers call. + """ + return pickle.loads(pickle.dumps(config, pickle.HIGHEST_PROTOCOL)) + + class ConfigManager: """ Reads and writes the main application configuration files. @@ -126,9 +146,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 +229,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 +272,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 +378,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: diff --git a/test/test_config_load_cache.py b/test/test_config_load_cache.py index e0d634fb5..3b44e364c 100644 --- a/test/test_config_load_cache.py +++ b/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.""" diff --git a/test/test_config_manager_secrets.py b/test/test_config_manager_secrets.py index da24a734f..dd6138b39 100644 --- a/test/test_config_manager_secrets.py +++ b/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( diff --git a/test/web_interface/test_plugin_config_endpoints.py b/test/web_interface/test_plugin_config_endpoints.py new file mode 100644 index 000000000..5009d49f2 --- /dev/null +++ b/test/web_interface/test_plugin_config_endpoints.py @@ -0,0 +1,172 @@ +"""GET and POST /plugins/config against a real ConfigManager and SchemaManager. + +Each class is one bug, reproduced through the endpoint the settings form and +API clients use, with assertions on config.json and config_secrets.json. +""" + +import json +from unittest.mock import MagicMock + +import pytest +from flask import Flask + +from src.config_manager import ConfigManager +from src.plugin_system.schema_manager import SchemaManager +from web_interface.blueprints.api_v3 import api_v3 + +PLUGIN_ID = "demo" +OTHER_ID = "other" + +SCHEMA = { + "type": "object", + "properties": { + "enabled": {"type": "boolean", "default": True}, + "api_key": {"type": "string", "x-secret": True, "default": ""}, + "city": {"type": "string", "default": "Austin"}, + "mqtt": { + "type": "object", + "properties": { + "host": {"type": "string", "default": ""}, + "port": {"type": "integer", "default": 1883, + "minimum": 1, "maximum": 65535}, + "password": {"type": "string", "x-secret": True, "default": ""}, + }, + }, + "accounts": { + "type": "array", + "default": [], + "items": { + "type": "object", + "properties": { + "name": {"type": "string"}, + "token": {"type": "string", "x-secret": True}, + }, + }, + }, + }, +} + +OTHER_SCHEMA = { + "type": "object", + "properties": { + "enabled": {"type": "boolean", "default": True}, + "label": {"type": "string", "default": "x"}, + }, +} + +STORED = { + PLUGIN_ID: {"enabled": True, "city": "Paris", + "mqtt": {"host": "broker", "port": 1883}, + "accounts": [{"name": "a"}, {"name": "b"}]}, + OTHER_ID: {"enabled": True, "label": "hello"}, +} + +STORED_SECRETS = { + PLUGIN_ID: {"api_key": "TOPSECRET", + "accounts": [{"token": "TOK-A"}, {"token": "TOK-B"}]}, +} + +_ATTRS = ('config_manager', 'plugin_catalog', 'plugin_store_manager', + 'saved_repositories_manager', 'schema_manager', + 'operation_queue', 'operation_history', 'cache_manager') + + +@pytest.fixture +def env(tmp_path): + config_file = tmp_path / "config.json" + secrets_file = tmp_path / "config_secrets.json" + plugins_dir = tmp_path / "plugins" + for plugin_id, schema in ((PLUGIN_ID, SCHEMA), (OTHER_ID, OTHER_SCHEMA)): + plugin_dir = plugins_dir / plugin_id + plugin_dir.mkdir(parents=True) + (plugin_dir / "config_schema.json").write_text(json.dumps(schema)) + (plugin_dir / "manifest.json").write_text(json.dumps({"id": plugin_id})) + config_file.write_text(json.dumps(STORED)) + secrets_file.write_text(json.dumps(STORED_SECRETS)) + + sentinel = object() + originals = {name: getattr(api_v3, name, sentinel) for name in _ATTRS} + + config_manager = ConfigManager(config_path=str(config_file), + secrets_path=str(secrets_file)) + config_manager.template_path = str(tmp_path / "no-template.json") + plugin_manager = MagicMock() + plugin_manager.plugin_manifests = {PLUGIN_ID: {"id": PLUGIN_ID}, + OTHER_ID: {"id": OTHER_ID}} + plugin_manager.plugins_dir = plugins_dir + + for name in _ATTRS: + setattr(api_v3, name, MagicMock()) + api_v3.config_manager = config_manager + api_v3.schema_manager = SchemaManager(plugins_dir=plugins_dir, project_root=tmp_path) + api_v3.plugin_catalog = plugin_manager + api_v3.operation_queue = None + + app = Flask(__name__) + app.config["TESTING"] = True + app.register_blueprint(api_v3, url_prefix="/api/v3") + + class Env: + client = app.test_client() + + @staticmethod + def use_schema(schema, plugin_id=PLUGIN_ID): + (plugins_dir / plugin_id / "config_schema.json").write_text(json.dumps(schema)) + + @staticmethod + def store(section, plugin_id=PLUGIN_ID): + main = json.loads(config_file.read_text()) + main[plugin_id] = section + config_file.write_text(json.dumps(main)) + + @staticmethod + def main(): + return json.loads(config_file.read_text()) + + @staticmethod + def secrets(): + return json.loads(secrets_file.read_text()) + + @staticmethod + def post_form(data, plugin_id=PLUGIN_ID): + return Env.client.post(f"/api/v3/plugins/config?plugin_id={plugin_id}", + data=data) + + @staticmethod + def post_json(config, plugin_id=PLUGIN_ID): + return Env.client.post("/api/v3/plugins/config", + json={"plugin_id": plugin_id, "config": config}) + + yield Env + + for name, original in originals.items(): + if original is sentinel: + if hasattr(api_v3, name): + delattr(api_v3, name) + else: + setattr(api_v3, name, original) + + +class TestARejectedSaveLeavesNothingBehind: + """The form save edited the cached config load_config hands out, then + failed validation. The cache kept the edit, and the next save of any + other setting wrote it to config.json -- the rejected value, and a + nested secret typed into the same form in plain text.""" + + REJECTED = {"mqtt.host": "broker", "mqtt.port": "99999", + "mqtt.password": "hunter2", "__rendered_section": ["mqtt"]} + + def test_the_rejected_values_never_reach_config_json(self, env): + assert env.post_form(self.REJECTED).status_code == 400 + + resp = env.post_json({"label": "bye"}, plugin_id=OTHER_ID) + assert resp.status_code == 200, resp.get_json() + + main = env.main() + assert main[OTHER_ID]["label"] == "bye" + assert main[PLUGIN_ID]["mqtt"] == {"host": "broker", "port": 1883} + assert "hunter2" not in json.dumps(main) + + def test_the_form_reloads_with_the_stored_values(self, env): + assert env.post_form(self.REJECTED).status_code == 400 + assert api_v3.config_manager.load_config()[PLUGIN_ID]["mqtt"]["port"] == 1883 From 2c54b80d891cf2999bd23cb8ee4a33c3cb6d6052 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 20:59:23 -0400 Subject: [PATCH 2/8] fix(web): GET /plugins/config masks secrets and refuses core sections The route returned the plugin's section as load_config() has it, with config_secrets.json merged in: API keys and tokens went out in plain text. #276 masked them here; #330's rewrite of the route dropped it, while the settings page and GET /config/secrets kept masking. It also took any plugin_id, so ?plugin_id=web_auth returned the login's cookie-signing key and password hash, and ?plugin_id=github the Plugin Store token, which GET /config/main strips and redacts. The route now refuses what _non_plugin_id_error refuses for reset and uninstall (core sections, malformed ids) with a 400, and blanks x-secret fields with mask_secret_fields after the defaults merge, as the page does. A plugin with no schema has its credential-named fields blanked by _redact_credentials, as GET /config/main does. Blank rather than the bullets of GET /config/secrets: the save drops a blank secret as "unchanged" (remove_empty_secrets) but would store the bullets, so the response must post back as it came. Tested: GET, then POST the response unchanged, keeps every stored secret. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 12 +++++ .../test_plugin_config_endpoints.py | 50 +++++++++++++++++++ .../blueprints/api_v3/plugin_config.py | 34 ++++++++++--- 3 files changed, 89 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd0ea5553..ef8adb96d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -545,6 +545,18 @@ policies are unchanged. reaches the cache unless it is saved. The copy is a pickle round trip: 2.0 ms for a real 64 KiB config on a Pi 4, against 6.9 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. - 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 diff --git a/test/web_interface/test_plugin_config_endpoints.py b/test/web_interface/test_plugin_config_endpoints.py index 5009d49f2..cd4a3e652 100644 --- a/test/web_interface/test_plugin_config_endpoints.py +++ b/test/web_interface/test_plugin_config_endpoints.py @@ -170,3 +170,53 @@ def test_the_rejected_values_never_reach_config_json(self, env): def test_the_form_reloads_with_the_stored_values(self, env): assert env.post_form(self.REJECTED).status_code == 400 assert api_v3.config_manager.load_config()[PLUGIN_ID]["mqtt"]["port"] == 1883 + + +class TestGetMasksSecrets: + """GET /plugins/config returned the section with config_secrets.json + merged in, secrets and all: the masking #276 added was lost when the + route was rewritten. The settings page and GET /config/secrets mask.""" + + def test_secrets_come_back_blank(self, env): + data = env.client.get(f"/api/v3/plugins/config?plugin_id={PLUGIN_ID}").get_json()["data"] + assert data["api_key"] == "" + assert data["accounts"] == [{"name": "a", "token": ""}, {"name": "b", "token": ""}] + assert data["city"] == "Paris" + + def test_posting_the_response_back_keeps_every_secret(self, env): + data = env.client.get(f"/api/v3/plugins/config?plugin_id={PLUGIN_ID}").get_json()["data"] + resp = env.post_json(data) + assert resp.status_code == 200, resp.get_json() + assert env.secrets()[PLUGIN_ID] == STORED_SECRETS[PLUGIN_ID] + assert "TOPSECRET" not in json.dumps(env.main()) + + def test_the_settings_form_posting_masked_fields_keeps_every_secret(self, env): + # The page renders secrets blank (pages_v3 masks the same way) + resp = env.post_form({ + "api_key": "", "city": "Lyon", "mqtt.host": "broker", "mqtt.port": "1883", + "mqtt.password": "", "__rendered_section": ["api_key", "city", "mqtt"]}) + assert resp.status_code == 200, resp.get_json() + assert env.secrets()[PLUGIN_ID] == STORED_SECRETS[PLUGIN_ID] + assert env.main()[PLUGIN_ID]["city"] == "Lyon" + + def test_a_plugin_without_a_schema_has_credential_named_fields_blanked(self, env, tmp_path): + (tmp_path / "plugins" / "bare").mkdir() + env.store({"enabled": True, "station": "KAUS"}, plugin_id="bare") + secrets = env.secrets() + secrets["bare"] = {"api_token": "BARE-TOKEN"} + (tmp_path / "config_secrets.json").write_text(json.dumps(secrets)) + data = env.client.get("/api/v3/plugins/config?plugin_id=bare").get_json()["data"] + assert data["api_token"] == "" + assert data["station"] == "KAUS" + + @pytest.mark.parametrize("section", ["web_auth", "github", "display"]) + def test_a_core_section_is_refused(self, env, tmp_path, section): + secrets = env.secrets() + secrets["web_auth"] = {"cookie_secret": "COOKIE-KEY", "password_hash": "HASH"} + secrets["github"] = {"api_token": "ghp_TOKEN"} + (tmp_path / "config_secrets.json").write_text(json.dumps(secrets)) + env.store({"hardware": {"rows": 32}}, plugin_id="display") + resp = env.client.get(f"/api/v3/plugins/config?plugin_id={section}") + assert resp.status_code == 400 + body = resp.get_data(as_text=True) + assert "COOKIE-KEY" not in body and "ghp_TOKEN" not in body diff --git a/web_interface/blueprints/api_v3/plugin_config.py b/web_interface/blueprints/api_v3/plugin_config.py index 8db8b1684..ae7d2cbe0 100644 --- a/web_interface/blueprints/api_v3/plugin_config.py +++ b/web_interface/blueprints/api_v3/plugin_config.py @@ -9,13 +9,15 @@ _enhance_schema_with_core_properties, _non_plugin_id_error, _filter_config_by_schema, _get_schema_property, _hidden_array_item_property, _plugin_directory, - _parse_form_value_with_schema, _schema_allows_null, _schema_type_is, - _set_missing_booleans_to_false, _set_nested_value, api_v3, datetime, - deep_merge, error_response, exception_error_response, find_secret_fields, - json, jsonify, logger, merge_secrets, os, remove_empty_secrets, request, - separate_secrets, success_response, validate_request_json, + _parse_form_value_with_schema, _redact_credentials, _schema_allows_null, + _schema_type_is, _set_missing_booleans_to_false, _set_nested_value, api_v3, + datetime, deep_merge, error_response, exception_error_response, + find_secret_fields, json, jsonify, logger, merge_secrets, os, + remove_empty_secrets, request, separate_secrets, success_response, + validate_request_json, ) from src.web_interface.config_arrays import coerce_array_shapes +from src.web_interface.secret_helpers import mask_secret_fields from src.web_interface.validators import dedup_unique_arrays import web_interface.blueprints.api_v3 as _pkg # Read through the module rather than bound by value: tests patch these @@ -43,6 +45,12 @@ def get_plugin_config(): context={'missing_params': ['plugin_id']}, status_code=400 ) + # load_config() merges config_secrets.json in, core sections + # included: ?plugin_id=web_auth returned the login's cookie key and + # password hash, and ?plugin_id=github the Plugin Store token. + id_error = _non_plugin_id_error(plugin_id) + if id_error: + return id_error # Get plugin configuration from config manager main_config = api_v3.config_manager.load_config() @@ -52,12 +60,13 @@ def get_plugin_config(): # missing fields, reading legacy booleans as objects first: what the # plugin runs with, and what posts back through the JSON save schema_mgr = api_v3.schema_manager + schema = None if schema_mgr: try: from src.plugin_system.schema_manager import prepare_plugin_config + schema = schema_mgr.load_schema(plugin_id, use_cache=True) defaults = schema_mgr.generate_default_config(plugin_id, use_cache=True) - plugin_config = prepare_plugin_config( - plugin_config, schema_mgr.load_schema(plugin_id, use_cache=True), defaults) + plugin_config = prepare_plugin_config(plugin_config, schema, defaults) except Exception as e: # Log but don't fail - defaults merge is best effort logger.warning("Could not merge defaults for %s: %s", plugin_id, e) @@ -158,6 +167,17 @@ def get_plugin_config(): 'display_duration': 30 } + # Secrets go out blank, as the settings page renders them (#276 added + # this; #330 dropped it). Blank, not the bullets GET /config/secrets + # uses: the save reads a blank secret as "unchanged", so this + # response posts back without erasing one. + properties = schema.get('properties') if isinstance(schema, dict) else None + if isinstance(properties, dict): + plugin_config = mask_secret_fields(plugin_config, properties) + else: + # No schema to mark them: blank whatever is named like one + plugin_config = _redact_credentials(plugin_config) + return success_response(data=plugin_config) except Exception as e: return exception_error_response(e, ErrorCode.CONFIG_LOAD_FAILED) From 033e4aa7bcba6105046e465f98d165e2be9acce0 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 21:01:09 -0400 Subject: [PATCH 3/8] fix(web): parse a table row's cells against the list's item schema An array of objects drawn as a table posts each cell as "cities.0.timezone". _get_schema_property stopped at "cities" (an array, not an object with properties), so _parse_form_value_with_schema got no schema for the cell and guessed: a blank optional text cell became None and a text cell holding digits became an int. Validation refused both, so every save of the page failed for as long as such a row existed -- geochron's city without a timezone, a countdown named "2027". A secret cell is always drawn blank, so a plugin with secrets in its rows could not be saved from the form at all. The lookup now steps from an index segment into the array's items: to the item schema itself for "color.2", into its properties for a row cell. Number, boolean and required cells convert as before. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 9 +++ test/web_interface/test_api_v3_helpers.py | 13 ++++ .../test_plugin_config_endpoints.py | 73 +++++++++++++++++++ web_interface/blueprints/api_v3/__init__.py | 13 ++++ 4 files changed, 108 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef8adb96d..5dbf9001e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -557,6 +557,15 @@ policies are unchanged. "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 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 diff --git a/test/web_interface/test_api_v3_helpers.py b/test/web_interface/test_api_v3_helpers.py index fbd142bfc..068e97e34 100644 --- a/test/web_interface/test_api_v3_helpers.py +++ b/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 diff --git a/test/web_interface/test_plugin_config_endpoints.py b/test/web_interface/test_plugin_config_endpoints.py index cd4a3e652..5771c30c4 100644 --- a/test/web_interface/test_plugin_config_endpoints.py +++ b/test/web_interface/test_plugin_config_endpoints.py @@ -220,3 +220,76 @@ def test_a_core_section_is_refused(self, env, tmp_path, section): assert resp.status_code == 400 body = resp.get_data(as_text=True) assert "COOKIE-KEY" not in body and "ghp_TOKEN" not in body + + +ROWS_SCHEMA = { + "type": "object", + "properties": { + "enabled": {"type": "boolean", "default": True}, + "cities": { + "type": "array", + "x-widget": "array-table", + "default": [], + "items": { + "type": "object", + "properties": { + "name": {"type": "string"}, + "timezone": {"type": "string"}, + "lat": {"type": "number"}, + "show": {"type": "boolean", "default": True}, + }, + "required": ["name", "lat"], + }, + }, + }, +} + + +class TestArrayRowCellsFollowTheItemSchema: + """A table row posts its cells as ``cities.0.timezone``. The schema + lookup stopped at the array, so each cell was parsed blind: a blank + optional text cell became null and a text cell holding digits became a + number, and either failed validation -- every save of the page, for as + long as the row existed (geochron's city without a timezone, a countdown + named "2027").""" + + ROW = {"cities.0.name": "Tokyo", "cities.0.timezone": "Asia/Tokyo", + "cities.0.lat": "35.68", "cities.0.show": "true", + "__rendered_section": ["cities"]} + + @pytest.fixture(autouse=True) + def _rows(self, env): + env.use_schema(ROWS_SCHEMA) + env.store({"enabled": True, "cities": [ + {"name": "Tokyo", "timezone": "Asia/Tokyo", "lat": 35.68, "show": True}]}) + + def test_a_blank_optional_text_cell_saves(self, env): + resp = env.post_form({**self.ROW, "cities.0.timezone": ""}) + assert resp.status_code == 200, resp.get_json() + assert env.main()[PLUGIN_ID]["cities"][0]["timezone"] == "" + + def test_a_text_cell_of_digits_stays_text(self, env): + resp = env.post_form({**self.ROW, "cities.0.name": "2027"}) + assert resp.status_code == 200, resp.get_json() + assert env.main()[PLUGIN_ID]["cities"][0]["name"] == "2027" + + def test_number_and_boolean_cells_still_convert(self, env): + resp = env.post_form({**self.ROW, "cities.0.show": "false"}) + assert resp.status_code == 200, resp.get_json() + assert env.main()[PLUGIN_ID]["cities"] == [ + {"name": "Tokyo", "timezone": "Asia/Tokyo", "lat": 35.68, "show": False}] + + +class TestMaskedSecretCellsInARow: + """The same lookup: a row's secret cell, rendered blank, came back as + null and failed validation, so a plugin with secrets in a list could not + be saved from its settings page at all.""" + + def test_the_stored_tokens_survive_a_save_of_the_form(self, env): + resp = env.post_form({ + "city": "Lyon", "accounts.0.name": "a", "accounts.0.token": "", + "accounts.1.name": "b", "accounts.1.token": "", + "__rendered_section": ["city", "accounts"]}) + assert resp.status_code == 200, resp.get_json() + assert env.secrets()[PLUGIN_ID] == STORED_SECRETS[PLUGIN_ID] + assert env.main()[PLUGIN_ID]["accounts"] == [{"name": "a"}, {"name": "b"}] diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index f8a903ab9..7fd0f57e0 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -957,6 +957,19 @@ def _get_schema_property(schema, key_path): i = j matched = True break + # Through an array to its items: a table row posts its cells + # as "cities.0.timezone", where the index names no property. + # Stopping here left each cell parsed with no schema at all, + # so a blank text cell became null and "2027" a number. + items = prop.get('items') if _schema_type_is(prop, 'array') else None + if isinstance(items, dict) and parts[j].isdigit(): + if j + 1 == len(parts): + return items + if 'properties' in items: + current = items['properties'] + i = j + 1 + matched = True + break # Matched a non-object before consuming the path — can't go deeper. return None if not matched: From b3461867af0b4f2795d835bf6b22c24ab8f84a46 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 21:02:39 -0400 Subject: [PATCH 4/8] fix(web): a blank secret field saves as "unchanged", required or not The settings page draws a stored secret blank (mask_secret_fields) and posts the blank back. _parse_form_value_with_schema turned a blank optional string into "" -- which the save drops as unchanged (remove_empty_secrets) -- but a blank required one into None. For a secret that is required with no default (youtube-stats' api_key) that None failed validation, so every save of the page was refused until the key was typed in again. A blank text secret (x-secret, type string) now parses to "", whatever its required list says; a list or object secret keeps getting [] or {}, which the save drops the same way. Not _SKIP_FIELD: skipping keeps the value load_config() merged in, and the save would then write it back to config_secrets.json -- after a secret change the cached section can still hold the old one, so that write reverted it. A test covers that sequence. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +++ .../test_plugin_config_endpoints.py | 49 +++++++++++++++++++ web_interface/blueprints/api_v3/__init__.py | 10 ++++ 3 files changed, 65 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dbf9001e..93b348834 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -566,6 +566,12 @@ policies are unchanged. 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. - 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 diff --git a/test/web_interface/test_plugin_config_endpoints.py b/test/web_interface/test_plugin_config_endpoints.py index 5771c30c4..12ec0aba5 100644 --- a/test/web_interface/test_plugin_config_endpoints.py +++ b/test/web_interface/test_plugin_config_endpoints.py @@ -293,3 +293,52 @@ def test_the_stored_tokens_survive_a_save_of_the_form(self, env): assert resp.status_code == 200, resp.get_json() assert env.secrets()[PLUGIN_ID] == STORED_SECRETS[PLUGIN_ID] assert env.main()[PLUGIN_ID]["accounts"] == [{"name": "a"}, {"name": "b"}] + + +class TestABlankSecretIsLeftAsStored: + """The form renders a secret blank and posts the blank back. For a + required secret with no default (youtube-stats' api_key) the blank was + read as null, failed validation, and blocked every save of the page + until the key was typed in again.""" + + @pytest.fixture(autouse=True) + def _required_secret(self, env): + schema = json.loads(json.dumps(SCHEMA)) + del schema["properties"]["api_key"]["default"] + schema["required"] = ["api_key"] + env.use_schema(schema) + + def test_saving_other_settings_keeps_the_stored_secret(self, env): + resp = env.post_form({"api_key": "", "city": "Lyon", + "__rendered_section": ["api_key", "city"]}) + assert resp.status_code == 200, resp.get_json() + assert env.main()[PLUGIN_ID]["city"] == "Lyon" + assert env.secrets()[PLUGIN_ID]["api_key"] == "TOPSECRET" + + def test_a_new_secret_is_still_saved(self, env): + resp = env.post_form({"api_key": "NEW-KEY", "city": "Lyon", + "__rendered_section": ["api_key", "city"]}) + assert resp.status_code == 200, resp.get_json() + assert env.secrets()[PLUGIN_ID]["api_key"] == "NEW-KEY" + + def test_a_changed_secret_then_left_blank_stays_changed(self, env): + # The second save must not write back what the first one's load + # had merged in (the old key) + env.post_form({"api_key": "NEW-KEY", "__rendered_section": ["api_key"]}) + resp = env.post_form({"api_key": "", "city": "Nice", + "__rendered_section": ["api_key", "city"]}) + assert resp.status_code == 200, resp.get_json() + assert env.secrets()[PLUGIN_ID]["api_key"] == "NEW-KEY" + + def test_a_blank_list_secret_is_left_as_stored_too(self, env, tmp_path): + schema = json.loads(json.dumps(SCHEMA)) + schema["properties"]["tokens"] = {"type": "array", "x-secret": True, + "items": {"type": "string"}, "default": []} + env.use_schema(schema) + secrets = env.secrets() + secrets[PLUGIN_ID]["tokens"] = ["t1", "t2"] + (tmp_path / "config_secrets.json").write_text(json.dumps(secrets)) + resp = env.post_form({"tokens": "", "city": "Lyon", + "__rendered_section": ["tokens", "city"]}) + assert resp.status_code == 200, resp.get_json() + assert env.secrets()[PLUGIN_ID]["tokens"] == ["t1", "t2"] diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 7fd0f57e0..296d941cb 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -1053,6 +1053,16 @@ def _parse_form_value_with_schema(value, key_path, schema): # Handle None/empty values if value is None or (isinstance(value, str) and value.strip() == ''): + # The form draws a stored secret blank, so a blank secret means + # "unchanged", and "" is what the save drops as unchanged + # (remove_empty_secrets). A required one with no default fell + # through to None below, failed validation, and blocked every save + # of the page until the secret was typed in again. Not _SKIP_FIELD: + # that keeps the merged value from load_config(), which the save + # would then write back to config_secrets.json. Text secrets only: + # a list or object one gets its empty value below, dropped the same. + if prop and prop.get('x-secret') and prop.get('type', 'string') == 'string': + return "" # A nullable field left blank means null, not an empty container. # This is the inherit sentinel for per-mode style overrides: an # empty list there would read as "the user chose no colour" rather From 16c0eb8f984b5083688dc2148d914ab40db62935 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 21:03:30 -0400 Subject: [PATCH 5/8] fix(web): POST /plugins/config refuses core sections and malformed ids Reset and uninstall check the plugin id with _non_plugin_id_error; the save did not. {"plugin_id": "display", "config": {...}} found no schema, so nothing was validated or filtered, and the body was merged into the core display section along with "enabled": true -- rows: "banana" included. A plugin_id that was not a string (a list, an object, a number) reached config.get() or the schema lookup, raised TypeError, and came back as a 500. Both the JSON and the form path now call _non_plugin_id_error first and answer its 400. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 ++++ .../test_plugin_config_endpoints.py | 23 +++++++++++++++++++ .../blueprints/api_v3/plugin_config.py | 9 ++++++++ 3 files changed, 37 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 93b348834..52d443437 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -572,6 +572,11 @@ policies are unchanged. 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 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 diff --git a/test/web_interface/test_plugin_config_endpoints.py b/test/web_interface/test_plugin_config_endpoints.py index 12ec0aba5..4ace90be4 100644 --- a/test/web_interface/test_plugin_config_endpoints.py +++ b/test/web_interface/test_plugin_config_endpoints.py @@ -342,3 +342,26 @@ def test_a_blank_list_secret_is_left_as_stored_too(self, env, tmp_path): "__rendered_section": ["tokens", "city"]}) assert resp.status_code == 200, resp.get_json() assert env.secrets()[PLUGIN_ID]["tokens"] == ["t1", "t2"] + + +class TestSaveRefusesWhatIsNotAPluginId: + """GET and reset refuse a core section or a malformed id; the save took + any of them. ``{"plugin_id": "display"}`` merged unvalidated values into + the core display section, and an id that was not a string raised a + TypeError, answered as a 500.""" + + def test_a_core_section_is_refused_and_left_alone(self, env): + env.store({"hardware": {"rows": 32}}, plugin_id="display") + resp = env.post_json({"hardware": {"rows": "banana"}}, plugin_id="display") + assert resp.status_code == 400 + assert env.main()["display"] == {"hardware": {"rows": 32}} + + def test_the_form_save_refuses_one_too(self, env): + resp = env.post_form({"password_hash": "x"}, plugin_id="web_auth") + assert resp.status_code == 400 + assert "web_auth" not in env.main() + + @pytest.mark.parametrize("plugin_id", [["demo"], {"id": "demo"}, 7, "", "../demo"]) + def test_a_malformed_id_is_a_400(self, env, plugin_id): + resp = env.post_json({"city": "Lyon"}, plugin_id=plugin_id) + assert resp.status_code == 400 diff --git a/web_interface/blueprints/api_v3/plugin_config.py b/web_interface/blueprints/api_v3/plugin_config.py index ae7d2cbe0..fddb2d17d 100644 --- a/web_interface/blueprints/api_v3/plugin_config.py +++ b/web_interface/blueprints/api_v3/plugin_config.py @@ -203,6 +203,12 @@ def save_plugin_config(): if error: return error plugin_id = data['plugin_id'] + # As reset and uninstall do: {"plugin_id": "display"} merged + # unvalidated values into the core display section, and an id + # that was not a string raised a TypeError, answered as a 500. + id_error = _non_plugin_id_error(plugin_id) + if id_error: + return id_error submitted_config = data.get('config', {}) if not isinstance(submitted_config, dict): return error_response( @@ -221,6 +227,9 @@ def save_plugin_config(): 'plugin_id required in query string', status_code=400 ) + id_error = _non_plugin_id_error(plugin_id) + if id_error: + return id_error # Load existing config as base (partial form updates should merge, not replace) existing_config = {} From ac61722f9aa0a0b2c2c246f92bc13ae478d8d5f3 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 21:04:53 -0400 Subject: [PATCH 6/8] fix(web): a text field keeps "true", "[1, 2]" and "{}" as typed _parse_form_value_with_schema guessed before it consulted the schema: "true"/"false" became booleans, and a value starting with "[" or "{" that parsed as JSON became a list or object, whatever the field's type. A text setting holding "true", "False", "[1, 2]" or "{}" was then refused by validation ("Expected type string, got bool"), and the save with it. A field whose schema type is string, or string-or-null, now returns the posted text as it came. Every other type goes through the conversions as before; numbers in text fields were already left alone. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +++++ .../test_plugin_config_endpoints.py | 26 +++++++++++++++++++ web_interface/blueprints/api_v3/__init__.py | 8 ++++++ 3 files changed, 40 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 52d443437..d1db3681c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -577,6 +577,12 @@ policies are unchanged. `{"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 diff --git a/test/web_interface/test_plugin_config_endpoints.py b/test/web_interface/test_plugin_config_endpoints.py index 4ace90be4..d1a523145 100644 --- a/test/web_interface/test_plugin_config_endpoints.py +++ b/test/web_interface/test_plugin_config_endpoints.py @@ -365,3 +365,29 @@ def test_the_form_save_refuses_one_too(self, env): def test_a_malformed_id_is_a_400(self, env, plugin_id): resp = env.post_json({"city": "Lyon"}, plugin_id=plugin_id) assert resp.status_code == 400 + + +class TestTextFieldsKeepWhatWasTyped: + """A text field holding "true", "False", "[1, 2]" or "{}" was converted + to a boolean, list or object before the schema's type was consulted, and + the save then failed validation for a perfectly good string.""" + + @pytest.mark.parametrize("typed", ["true", "False", "[1, 2]", "{}", "42"]) + def test_a_text_field(self, env, typed): + resp = env.post_form({"city": typed, "__rendered_section": ["city"]}) + assert resp.status_code == 200, resp.get_json() + assert env.main()[PLUGIN_ID]["city"] == typed + + def test_a_nullable_text_field(self, env): + schema = json.loads(json.dumps(SCHEMA)) + schema["properties"]["nickname"] = {"type": ["string", "null"], "default": None} + env.use_schema(schema) + resp = env.post_form({"nickname": "false", "__rendered_section": ["nickname"]}) + assert resp.status_code == 200, resp.get_json() + assert env.main()[PLUGIN_ID]["nickname"] == "false" + + def test_other_types_still_convert(self, env): + resp = env.post_form({"mqtt.host": "true", "mqtt.port": "8883", + "__rendered_section": ["mqtt"]}) + assert resp.status_code == 200, resp.get_json() + assert env.main()[PLUGIN_ID]["mqtt"] == {"host": "true", "port": 8883} diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index 296d941cb..b40105beb 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -1097,6 +1097,14 @@ def _parse_form_value_with_schema(value, key_path, schema): if isinstance(value, str): stripped = value.strip() + # A text field keeps what was typed. The guesses below ran first, so + # "true", "False", "[1, 2]" or "{}" in a text field became a boolean, + # list or object, and the save failed validation for a good string. + declared = prop.get('type') if isinstance(prop, dict) else None + if declared == 'string' or (isinstance(declared, list) and + [t for t in declared if t != 'null'] == ['string']): + return value + # Check for boolean strings if stripped.lower() == 'true': return True From 867c49561b1c0af5026da8572343b9029d27b000 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 22:15:09 -0400 Subject: [PATCH 7/8] fix(config): copy the cached config without pickle _private_copy was a pickle round trip. It only ever unpickled bytes it had just made from our own dict, so nothing untrusted reached it, but it put pickle in the config path and Codacy failed the PR for it (B301/B403). The config is JSON data, so copying its dicts and lists is a full copy; every other value is immutable. Measured on ledpi (Pi 4) with its real 60 KiB config: 2.11 ms, against 1.92 ms for pickle and 6.75 ms for copy.deepcopy. Co-Authored-By: Claude Opus 5.5 --- src/config_manager.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/config_manager.py b/src/config_manager.py index cd70bd0fb..25911d2a8 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -31,7 +31,6 @@ import json import os import logging -import pickle from pathlib import Path from typing import Dict, Any, Optional, List from src.core_config_keys import CORE_CONFIG_KEYS, CORE_SECRETS_KEYS @@ -59,11 +58,21 @@ def _private_copy(config: Dict[str, Any]) -> Dict[str, Any]: plain text, since it had never reached config_secrets.json to be stripped. - A pickle round trip rather than copy.deepcopy: the config is plain JSON - data, and on a Pi 4 with a real 64 KiB config this takes 2.0 ms against - deepcopy's 6.9 ms, on a path ~30 handlers call. + 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 pickle.loads(pickle.dumps(config, pickle.HIGHEST_PROTOCOL)) + 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: From 01f0ae6ebe7dfb828d962b6329c1c9e112bef7d5 Mon Sep 17 00:00:00 2001 From: Chuck <33324927+ChuckBuilds@users.noreply.github.com> Date: Sat, 3 Oct 2026 22:15:21 -0400 Subject: [PATCH 8/8] docs(changelog): describe the config copy without pickle Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1db3681c..8a4386ada 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -542,9 +542,9 @@ policies are unchanged. 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 is a pickle round trip: - 2.0 ms for a real 64 KiB config on a Pi 4, against 6.9 ms for - `copy.deepcopy`. + 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