diff --git a/CHANGELOG.md b/CHANGELOG.md index cc9a0c8d..52e08dcb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -618,6 +618,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 diff --git a/src/config_manager.py b/src/config_manager.py index dd7982a3..25911d2a 100644 --- a/src/config_manager.py +++ b/src/config_manager.py @@ -46,6 +46,35 @@ from src.common.permission_utils import ( 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 @@ class ConfigManager: 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 @@ class ConfigManager: 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 @@ class ConfigManager: # 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 @@ class ConfigManager: 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 e0d634fb..3b44e364 100644 --- a/test/test_config_load_cache.py +++ b/test/test_config_load_cache.py @@ -58,7 +58,8 @@ class TestFastPath: 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 @@ class TestFastPath: 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 @@ class TestSaveCoherence: 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 da24a734..dd6138b3 100644 --- a/test/test_config_manager_secrets.py +++ b/test/test_config_manager_secrets.py @@ -143,7 +143,10 @@ class TestLoadFastPath: 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_api_v3_helpers.py b/test/web_interface/test_api_v3_helpers.py index fbd142bf..068e97e3 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 @@ class TestGetSchemaProperty: 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 new file mode 100644 index 00000000..d1a52314 --- /dev/null +++ b/test/web_interface/test_plugin_config_endpoints.py @@ -0,0 +1,393 @@ +"""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 + + +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 + + +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"}] + + +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"] + + +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 + + +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 f8a903ab..b40105be 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: @@ -1040,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 @@ -1074,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 diff --git a/web_interface/blueprints/api_v3/plugin_config.py b/web_interface/blueprints/api_v3/plugin_config.py index 8db8b168..fddb2d17 100644 --- a/web_interface/blueprints/api_v3/plugin_config.py +++ b/web_interface/blueprints/api_v3/plugin_config.py @@ -9,13 +9,15 @@ from web_interface.blueprints.api_v3 import ( _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) @@ -183,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( @@ -201,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 = {}