diff --git a/test/test_api_v3_font_upload_size.py b/test/test_api_v3_font_upload_size.py new file mode 100644 index 00000000..cd1b731d --- /dev/null +++ b/test/test_api_v3_font_upload_size.py @@ -0,0 +1,60 @@ +"""Regression test: POST /fonts/upload must enforce its own stated size limit. + +validate_file_upload(filename, max_size_mb=10, allowed_extensions=[...]) reads +like it checks the upload's size, but it only ever validated the filename +(traversal characters, extension) -- max_size_mb was accepted and silently +ignored. Nothing else in the handler checked the actual upload size either, +so it saved whatever was posted to assets/fonts/ regardless of +size, unlike the sibling .star and plugin-asset upload routes, which check +`file.tell()` against a stated limit before saving. +""" + +import io +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + +URL = "/api/v3/fonts/upload" +TEN_MB = 10 * 1024 * 1024 + + +@pytest.fixture +def fonts_root(tmp_path): + # PROJECT_ROOT is bound by value in fonts.py, so it is patched there. + with patch("web_interface.blueprints.api_v3.fonts.PROJECT_ROOT", tmp_path): + yield tmp_path + + +def upload(client, content, filename="myfont.ttf", family="myfont"): + data = { + "font_file": (io.BytesIO(content), filename), + "font_family": family, + } + return client.post(URL, data=data, content_type="multipart/form-data") + + +class TestFontUploadSizeLimit: + def test_oversized_font_is_rejected(self, api_v3_client, fonts_root): + response = upload(api_v3_client, b"x" * (TEN_MB + 1)) + assert response.status_code == 400 + assert "too large" in response.get_json()["message"].lower() + assert not (fonts_root / "assets" / "fonts" / "myfont.ttf").exists(), ( + "an oversized font was saved to disk before being rejected") + + def test_a_font_right_at_the_limit_is_accepted(self, api_v3_client, fonts_root): + response = upload(api_v3_client, b"x" * TEN_MB, + filename="atlimit.ttf", family="atlimit") + assert response.status_code == 200, response.get_json() + assert (fonts_root / "assets" / "fonts" / "atlimit.ttf").exists() + + def test_an_ordinary_small_font_is_still_accepted(self, api_v3_client, fonts_root): + response = upload(api_v3_client, b"fake font bytes", + filename="small.ttf", family="small") + assert response.status_code == 200, response.get_json() + assert (fonts_root / "assets" / "fonts" / "small.ttf").read_bytes() == b"fake font bytes" diff --git a/test/test_api_v3_on_demand_restart.py b/test/test_api_v3_on_demand_restart.py new file mode 100644 index 00000000..c203fbe4 --- /dev/null +++ b/test/test_api_v3_on_demand_restart.py @@ -0,0 +1,93 @@ +"""Regression test: POST /display/on-demand/start restarting a running +service must not import a name that does not exist. + +display.py has `import web_interface.blueprints.api_v3 as _pkg` and reads +mutable, test-patched attributes back through it (`_pkg.time.time()`, +`_pkg._get_starlark_plugin()`, ...) rather than binding them by value, per +the package's own docstring. One spot went further and wrote a genuine +`import` *statement* against that alias -- + + import _pkg.time as time_module + +-- but `_pkg` is a local name bound by `import ... as _pkg` in this module, +not a real top-level package, so `import _pkg.time` is not something Python +can resolve; it raises ModuleNotFoundError. That line only runs when the +display service is already running and the caller also asked to (re)start +it, so this endpoint failed on exactly the restart path -- the one where a +cache write recording the new on-demand request had already happened. + +The route wraps its body in `except Exception`, so the failure reached the +caller as a handled 500 with a generic message, not an unhandled crash -- +but a 500 all the same on a request that should have restarted the service +and reported success. +""" + +import sys +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + +URL = "/api/v3/display/on-demand/start" + + +@pytest.fixture +def restart_path(api_v3_module): + """Force the `service_was_running and start_service` branch. + + plugin_manager and config_manager are set to None so the route takes + the simplest path to that branch rather than tripping over unrelated + MagicMock plumbing; _ensure_cache_manager, _get_display_service_status, + _stop_display_service and _ensure_display_service_running are bound by + value in display.py (see its own docstring), so they are patched on + that submodule rather than on the package. + """ + api_v3_module.api_v3.plugin_manager = None + api_v3_module.api_v3.config_manager = None + + with patch("web_interface.blueprints.api_v3.display._ensure_cache_manager") as ensure_cache, \ + patch("web_interface.blueprints.api_v3.display._get_display_service_status") as get_status, \ + patch("web_interface.blueprints.api_v3.display._stop_display_service") as stop_service, \ + patch("web_interface.blueprints.api_v3.display._ensure_display_service_running") as ensure_running: + ensure_cache.return_value = MagicMock() + # Active before the request: service_was_running becomes True. + get_status.return_value = {"active": True} + ensure_running.return_value = {"active": True} + yield { + "ensure_cache": ensure_cache, + "get_status": get_status, + "stop_service": stop_service, + "ensure_running": ensure_running, + } + + +class TestRestartingARunningService: + def test_it_does_not_500(self, api_v3_client, restart_path): + response = api_v3_client.post( + URL, json={"plugin_id": "weather", "start_service": True}) + body = response.get_json() + assert response.status_code == 200, body + assert body["status"] == "success", body + + def test_the_service_is_actually_stopped_and_restarted( + self, api_v3_client, restart_path): + api_v3_client.post( + URL, json={"plugin_id": "weather", "start_service": True}) + restart_path["stop_service"].assert_called_once() + restart_path["ensure_running"].assert_called_once() + + def test_a_service_that_was_not_running_is_not_stopped_first( + self, api_v3_client, restart_path): + # The buggy import sits inside `if service_was_running and + # start_service`, so it only ever fired on the restart path -- + # this is the other side of that branch, unaffected either way, + # kept here so the branch condition itself stays covered. + restart_path["get_status"].return_value = {"active": False} + response = api_v3_client.post( + URL, json={"plugin_id": "weather", "start_service": True}) + assert response.status_code == 200, response.get_json() + restart_path["stop_service"].assert_not_called() diff --git a/test/test_api_v3_schedule_error_messages.py b/test/test_api_v3_schedule_error_messages.py new file mode 100644 index 00000000..383ec172 --- /dev/null +++ b/test/test_api_v3_schedule_error_messages.py @@ -0,0 +1,54 @@ +"""Regression test: per-day schedule validation errors must say "time", not +"_pkg.time". + +The split into a package rewrote every bare `time` reference that needed to +read through the shared module as `_pkg.time` (see the package's own +docstring on why -- tests patch it, so it has to be read back live rather +than bound by value). That rewrite was mechanical and matched the substring +"time" inside string literals and comments too, so the user-facing message + + "Invalid start time for {day}: ..." + +came out as + + "Invalid start _pkg.time for {day}: ..." + +in both POST /config/schedule and POST /config/dim-schedule, for both the +start and end time of a per-day entry. +""" + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from test._api_v3_test_helpers import api_v3_client, api_v3_module # noqa: F401,E402 + + +@pytest.mark.parametrize("url", [ + "/api/v3/config/schedule", + "/api/v3/config/dim-schedule", +]) +class TestPerDayTimeErrorsAreNotCorrupted: + def test_invalid_start_time_message(self, api_v3_client, api_v3_module, url): + response = api_v3_client.post(url, json={ + "mode": "per_day", + "monday_start": "not-a-time", + }) + assert response.status_code == 400 + message = response.get_json()["message"] + assert "_pkg" not in message, message + assert message.startswith("Invalid start time for monday:"), message + + def test_invalid_end_time_message(self, api_v3_client, api_v3_module, url): + response = api_v3_client.post(url, json={ + "mode": "per_day", + "monday_start": "07:00", + "monday_end": "not-a-time", + }) + assert response.status_code == 400 + message = response.get_json()["message"] + assert "_pkg" not in message, message + assert message.startswith("Invalid end time for monday:"), message diff --git a/test/test_api_v3_wifi_endpoints.py b/test/test_api_v3_wifi_endpoints.py index 0c713ae2..0b4a81ea 100644 --- a/test/test_api_v3_wifi_endpoints.py +++ b/test/test_api_v3_wifi_endpoints.py @@ -229,6 +229,61 @@ class TestRadio: assert api_v3_client.post(self.URL, json={"enabled": True}).status_code == 500 +class TestAutoEnableApMode: + URL = "/api/v3/wifi/ap/auto-enable" + + def test_requires_the_field(self, api_v3_client, wifi_manager): + response = api_v3_client.post(self.URL, json={}) + assert response.status_code == 400 + + @pytest.mark.parametrize("raw,expected", [ + (True, True), (False, False), + ("true", True), ("True", True), ("1", True), ("yes", True), + ("false", False), ("False", False), ("0", False), ("no", False), + (1, True), (0, False), + ]) + def test_value_is_coerced_not_just_truthy( + self, api_v3_client, wifi_manager, raw, expected): + # Regression: bool(data['auto_enable_ap_mode']) meant a caller who + # sent the JSON string "false" got it stored as True — bool("false") + # is True, since any non-empty string is truthy. + wifi_manager.config = {} + response = api_v3_client.post(self.URL, json={"auto_enable_ap_mode": raw}) + assert response.status_code == 200, response.get_json() + assert response.get_json()["data"]["auto_enable_ap_mode"] is expected + assert wifi_manager.config["auto_enable_ap_mode"] is expected + + def test_a_string_false_does_not_enable_it(self, api_v3_client, wifi_manager): + # The exact shape of the bug. + wifi_manager.config = {} + api_v3_client.post(self.URL, json={"auto_enable_ap_mode": "false"}) + assert wifi_manager.config["auto_enable_ap_mode"] is False + + +class TestRadioEnabledAndForceAcceptIntegers: + """`{"enabled": 1}` / `{"enabled": 0}` used to be mishandled: the old + coercion was `raw is True or (isinstance(raw, str) and ...)`, and + `1 is True` is False in Python -- an int is never the `True` singleton + even though it equals it -- so a plain integer fell through to False + regardless of its value. + """ + URL = "/api/v3/wifi/radio" + + @pytest.mark.parametrize("raw,expected", [(1, True), (0, False)]) + def test_enabled_as_an_integer(self, api_v3_client, wifi_manager, raw, expected): + wifi_manager.set_wifi_radio.return_value = (True, "ok", None) + wifi_manager.get_wifi_radio_state.return_value = {} + api_v3_client.post(self.URL, json={"enabled": raw}) + wifi_manager.set_wifi_radio.assert_called_once_with(expected, force=False) + + @pytest.mark.parametrize("raw,expected", [(1, True), (0, False)]) + def test_force_as_an_integer(self, api_v3_client, wifi_manager, raw, expected): + wifi_manager.set_wifi_radio.return_value = (True, "ok", None) + wifi_manager.get_wifi_radio_state.return_value = {} + api_v3_client.post(self.URL, json={"enabled": True, "force": raw}) + wifi_manager.set_wifi_radio.assert_called_once_with(True, force=expected) + + class TestNoRealNetworking: def test_wifi_manager_is_never_constructed_for_real(self, api_v3_client): # Guard against a future refactor moving the import to module level, diff --git a/test/test_config_main_redacts_secrets.py b/test/test_config_main_redacts_secrets.py index c33ab962..191f8d20 100644 --- a/test/test_config_main_redacts_secrets.py +++ b/test/test_config_main_redacts_secrets.py @@ -103,6 +103,43 @@ def test_non_dict_input_passes_through(): assert _redact_credentials(None) is None +def test_a_list_of_bare_credentials_is_redacted(): + """A credential-named key whose value is a list of scalars, not a list + of named-field dicts, used to pass through untouched: the dict branch + recursed into `v` on its own value only, so a bare string list item had + no field name to test and fell through the base case unredacted. + """ + config = {"tokens": ["abc123", "def456"], "timezone": "America/New_York"} + out = _redact_credentials(config) + assert out["tokens"] == ["", ""] + assert out["timezone"] == "America/New_York" + + +def test_a_list_nested_inside_a_credential_named_container_is_also_blanked(): + """The list fix applies at any depth under a credential-named key, not + only when the list is the key's direct value. + """ + config = {"auth": {"backup_tokens": ["a", "b"], "note": "keep"}} + out = _redact_credentials(config) + assert out["auth"]["backup_tokens"] == ["", ""] + assert out["auth"]["note"] == "keep" + + +def test_credential_shaped_container_with_named_fields_still_only_blanks_those(): + """Unchanged behaviour for the case the container test above already + covers: a dict whose sub-keys are semantically named fields is walked + normally, not blanket-blanked, so an ordinary field next to a + credential-named one survives. + """ + config = {"secrets": {"api_key": "k", "note": "keep", "list": ["a", "b"]}} + out = _redact_credentials(config) + assert out["secrets"]["api_key"] == "" + assert out["secrets"]["note"] == "keep" + # "list" isn't itself credential-named, so it is walked, not blanked -- + # but it holds no credential-named field either, so it survives whole. + assert out["secrets"]["list"] == ["a", "b"] + + def test_the_endpoint_itself_redacts(): """Through the view function, not the helper. diff --git a/test/web_interface/test_api_v3_backup_restore.py b/test/web_interface/test_api_v3_backup_restore.py index 1ea8d4cc..52758fba 100644 --- a/test/web_interface/test_api_v3_backup_restore.py +++ b/test/web_interface/test_api_v3_backup_restore.py @@ -140,6 +140,41 @@ class TestRequestValidation: assert restore.call_args[0][2].restore_config is True +class TestOptionsAreBooleanAware: + """Regression: bool("false") is True in Python. + + Every restore flag used bare bool() coercion, so a caller that sends its + options as JSON strings rather than real booleans -- a form field, a + hand-built request -- had `{"restore_secrets": "false"}` restore secrets + anyway, the opposite of what was asked. Fixed with the same + string-aware `_coerce_to_bool` already used for checkbox-style config + fields elsewhere in this package (config.py, plugins.py). + """ + + @pytest.mark.parametrize("raw,expected", [ + ("false", False), ("False", False), ("FALSE", False), + ("0", False), + ("true", True), ("True", True), ("1", True), + ]) + def test_string_valued_flags_are_parsed_not_just_truthy( + self, client, restore, raw, expected): + post(client, options=json.dumps({"restore_secrets": raw})) + assert restore.call_args[0][2].restore_secrets is expected + + def test_a_string_false_does_not_restore_secrets(self, client, restore): + # The exact shape of the bug: a truthy non-empty string coerced by + # bare bool() to True regardless of its contents. + post(client, options=json.dumps({"restore_secrets": "false"})) + assert restore.call_args[0][2].restore_secrets is False + + def test_real_json_booleans_still_work(self, client, restore): + post(client, options=json.dumps({"restore_secrets": False, + "restore_config": True})) + options = restore.call_args[0][2] + assert options.restore_secrets is False + assert options.restore_config is True + + class TestSuccess: def test_success_returns_the_result(self, client, restore): restore.return_value = FakeResult(success=True, restored=["config", "secrets"]) diff --git a/test/web_interface/test_calendar_oauth_endpoints.py b/test/web_interface/test_calendar_oauth_endpoints.py index b0293a28..ef18b413 100644 --- a/test/web_interface/test_calendar_oauth_endpoints.py +++ b/test/web_interface/test_calendar_oauth_endpoints.py @@ -363,6 +363,22 @@ class TestDiagnosticsAreRedacted: assert 'hunter2' not in error, error assert '' in error, error + def test_script_stderr_is_redacted_in_the_log_too(self, tmp_path, caplog): + # Regression: the return value went through redact_text (asserted + # above), but the logger.error call right next to it logged `raw` + # verbatim -- a script that handles OAuth client secrets and can + # quote them in its stderr, landing unredacted in the log (CWE-532). + script = tmp_path / 'calendar_registration.py' + script.write_text( + 'import sys\n' + 'sys.stderr.write("boom client_secret=hunter2 more\\n")\n', + encoding='utf-8') + with caplog.at_level('ERROR', logger=mod.logger.name): + mod._run_calendar_registration(tmp_path, '') + logged = '\n'.join(r.getMessage() for r in caplog.records) + assert 'hunter2' not in logged, logged + assert '' in logged, logged + def test_a_failing_script_payload_is_redacted(self, client): (client.plugin_dir / 'credentials.json').write_text('{}', encoding='utf-8') (client.plugin_dir / 'calendar_registration.py').write_text( diff --git a/test/web_interface/test_starlark_pixlet_routes.py b/test/web_interface/test_starlark_pixlet_routes.py index 55f53f9b..e1c09a86 100644 --- a/test/web_interface/test_starlark_pixlet_routes.py +++ b/test/web_interface/test_starlark_pixlet_routes.py @@ -395,6 +395,132 @@ class TestTheManifestStaysRelocatable: assert self._install(tmp_path)['star_file'] == 'demo.star' +class TestManifestLockPreventsLostUpdates: + """The standalone manifest fallback (no plugin instance loaded) reads, + mutates and writes manifest.json with no coordination across requests. + Each write is atomic on its own (temp file + rename), but two concurrent + read-modify-write cycles can still race: both read the same starting + manifest, and the second write silently discards whatever the first one + added. _starlark_manifest_lock closes that window -- mirrors + StarlarkAppsPlugin._update_manifest_safe, which already does this when + the plugin instance is loaded. + """ + + @pytest.fixture + def starlark_dir(self, tmp_path, monkeypatch): + from web_interface.blueprints import api_v3 as module + apps_dir = tmp_path / "starlark-apps" + apps_dir.mkdir() + monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir) + monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json') + module._write_starlark_manifest({'apps': {}}) + return apps_dir + + def test_two_concurrent_updates_are_both_kept(self, starlark_dir): + import threading + import time as _time + + from web_interface.blueprints import api_v3 as module + + def add_app(app_id): + with module._starlark_manifest_lock(): + manifest = module._read_starlark_manifest() + # Widen the window between read and write. Without the lock + # both threads read here before either writes, and whichever + # writes second overwrites the other's addition; with the + # lock, the second thread cannot even start its read until + # the first has written and released. + _time.sleep(0.05) + manifest.setdefault('apps', {})[app_id] = {'enabled': True} + module._write_starlark_manifest(manifest) + + threads = [threading.Thread(target=add_app, args=(app_id,)) + for app_id in ('a', 'b')] + for t in threads: + t.start() + for t in threads: + t.join(timeout=5) + + manifest = module._read_starlark_manifest() + assert set(manifest['apps']) == {'a', 'b'}, ( + "a concurrent update was lost: %r" % (manifest,)) + + def test_the_lock_is_reentrant_safe_across_sequential_calls(self, starlark_dir): + """Not reentrant within one thread -- just that using it twice in a + row (the ordinary case: one request, then the next) works cleanly + and does not leak the lock file descriptor or leave it locked.""" + from web_interface.blueprints import api_v3 as module + + for app_id in ('first', 'second'): + with module._starlark_manifest_lock(): + manifest = module._read_starlark_manifest() + manifest.setdefault('apps', {})[app_id] = {'enabled': True} + module._write_starlark_manifest(manifest) + + manifest = module._read_starlark_manifest() + assert set(manifest['apps']) == {'first', 'second'} + + +class TestConfigAndManifestStayInSync: + """Standalone-mode PUT /starlark/apps//config (no plugin instance + loaded) writes config.json and then the manifest. If the manifest write + fails after config.json was already written, the two disagree about + what was saved unless config.json is rolled back. + """ + + @pytest.fixture + def app_dir(self, tmp_path, monkeypatch): + from web_interface.blueprints import api_v3 as module + apps_dir = tmp_path / "starlark-apps" + apps_dir.mkdir() + monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir) + monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json') + one_app_dir = apps_dir / 'demo' + one_app_dir.mkdir() + module._write_starlark_manifest({'apps': {'demo': {'name': 'Demo', 'enabled': True}}}) + return one_app_dir + + def test_manifest_write_failure_rolls_back_an_existing_config_json(self, client, app_dir): + config_file = app_dir / 'config.json' + config_file.write_text(json.dumps({'existing': 'value'})) + + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \ + patch('web_interface.blueprints.api_v3._write_starlark_manifest', return_value=False): + resp = client.put('/api/v3/starlark/apps/demo/config', + json={'new_field': 'x'}) + + assert resp.status_code == 500 + assert json.loads(config_file.read_text()) == {'existing': 'value'}, ( + "config.json kept the new value even though the manifest write " + "that was supposed to follow it failed") + + def test_manifest_write_failure_removes_a_freshly_created_config_json(self, client, app_dir): + config_file = app_dir / 'config.json' + assert not config_file.exists() + + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \ + patch('web_interface.blueprints.api_v3._write_starlark_manifest', return_value=False): + resp = client.put('/api/v3/starlark/apps/demo/config', + json={'new_field': 'x'}) + + assert resp.status_code == 500 + assert not config_file.exists(), ( + "config.json was left behind even though the manifest write " + "that was supposed to follow it failed") + + def test_success_updates_both_config_and_manifest(self, client, app_dir): + from web_interface.blueprints import api_v3 as module + + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None): + resp = client.put('/api/v3/starlark/apps/demo/config', + json={'new_field': 'x'}) + + assert resp.status_code == 200, resp.get_json() + assert json.loads((app_dir / 'config.json').read_text())['new_field'] == 'x' + manifest = json.loads(module._STARLARK_MANIFEST_FILE.read_text()) + assert manifest['apps']['demo']['config']['new_field'] == 'x' + + # --------------------------------------------------------------------------- # The store loaded, then stopped loading, and nothing anywhere said why. # diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index ef172165..ea08a6c0 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -16,6 +16,8 @@ The route modules are imported at the *bottom*: they import names from here, so everything they need has to exist first. """ from flask import Blueprint, request, jsonify, Response +import contextlib +import fcntl import json import signal import contextlib @@ -296,12 +298,31 @@ def _redact_credentials(value): that round-trips this response cannot erase a secret it never saw. """ if isinstance(value, dict): - return {k: ("" if _looks_like_a_credential(k) and not isinstance(v, (dict, list)) + return {k: (_blank_credential_value(v) if _looks_like_a_credential(k) else _redact_credentials(v)) for k, v in value.items()} if isinstance(value, list): return [_redact_credentials(item) for item in value] return value +def _blank_credential_value(value): + """Blank a value that sits under a credential-shaped key. + + A dict is still walked -- `secrets: {api_key: ..., note: ...}` is a + section name, not a value to blank in one go, so sub-fields that are not + themselves credential-named survive (see + test_a_credential_shaped_container_is_still_walked). A scalar list item + has no field name to test, though: `_redact_credentials` previously + recursed into a list the same way it recurses into a dict, and a scalar + is returned unchanged by the base case, so a credential-named key whose + value was a bare list of secrets (e.g. `tokens: ["a", "b"]`) passed + through with nothing redacted. Blank every scalar reached under this key, + at any depth, instead. + """ + if isinstance(value, dict): + return _redact_credentials(value) + if isinstance(value, list): + return [_blank_credential_value(item) for item in value] + return "" def _validate_time_format(time_str): """Validate time format is HH:MM""" try: @@ -1235,12 +1256,11 @@ def _run_calendar_registration(plugin_dir: Path, stdin_payload: str): return payload, None raw = (result.stderr or result.stdout or '').strip() - # The unredacted text goes to the log, where it is worth having in full. - # What comes back over HTTP is redacted: this is a script that handles - # OAuth client secrets, and its stderr can quote them. + # Redacted in the log too: this is a script that handles OAuth client + # secrets, and its stderr can quote them verbatim (CWE-532). if raw: logger.error('calendar_registration.py failed (exit %s): %s', - result.returncode, raw) + result.returncode, redact_text(raw)) return None, 'Authentication script produced no result%s' % ( ': %s' % redact_text(raw) if raw else '') def _resolve_backup_export_dir() -> Path: @@ -1323,6 +1343,31 @@ def _find_pixlet_binary(explicit_path: Optional[str] = None) -> Optional[str]: return str(bundled) logger.warning("Pixlet bundled binary still not executable after chmod (%s); falling back to PATH", bundled) return shutil.which("pixlet") +@contextlib.contextmanager +def _starlark_manifest_lock(): + """Hold an exclusive lock across a standalone starlark-manifest read-modify-write. + + StarlarkAppsPlugin._update_manifest_safe (plugin-repos/starlark-apps/manager.py) + already does this -- fcntl.flock held for the whole read-modify-write cycle -- + when the plugin instance is loaded. These routes fall back to reading and + writing manifest.json directly when it is not, and did so with no lock: each + _write_starlark_manifest() call is atomic on its own (temp file + rename), but + two concurrent requests can each read the manifest, mutate their own copy, and + write it back, and the second write silently discards the first's change. + + Callers should do their read, mutation and _write_starlark_manifest() call + entirely inside the `with` block, mirroring the plugin's lock scope. + """ + _STARLARK_APPS_DIR.mkdir(parents=True, exist_ok=True) + lock_fd = os.open(str(_STARLARK_MANIFEST_FILE), os.O_CREAT | os.O_RDWR, 0o644) + try: + fcntl.flock(lock_fd, fcntl.LOCK_EX) + try: + yield + finally: + fcntl.flock(lock_fd, fcntl.LOCK_UN) + finally: + os.close(lock_fd) def _read_starlark_manifest() -> Dict[str, Any]: """Read the starlark-apps manifest.json directly from disk.""" try: @@ -1665,22 +1710,23 @@ def _install_star_file(app_id: str, star_file_path: str, metadata: Dict[str, Any with open(config_path, 'w') as f: json.dump(default_config, f, indent=2) - manifest = _read_starlark_manifest() - manifest.setdefault('apps', {})[app_id] = { - 'name': metadata.get('name', app_id), - 'enabled': True, - 'render_interval': metadata.get('render_interval', 300), - 'display_duration': metadata.get('display_duration', 15), - 'config': metadata.get('config', {}), - # The filename, not the full path. Readers join this to the app's own - # directory and fall back to a bare '.star', so an absolute - # value gave the key two meanings -- and Path.__truediv__ discards the - # left side when the right is absolute, which pinned the manifest to - # whatever PROJECT_ROOT installed it. Moving or redeploying the install - # then left the app unable to find its own file. - 'star_file': dest.name, - } - return _write_starlark_manifest(manifest) + with _starlark_manifest_lock(): + manifest = _read_starlark_manifest() + manifest.setdefault('apps', {})[app_id] = { + 'name': metadata.get('name', app_id), + 'enabled': True, + 'render_interval': metadata.get('render_interval', 300), + 'display_duration': metadata.get('display_duration', 15), + 'config': metadata.get('config', {}), + # The filename, not the full path. Readers join this to the app's own + # directory and fall back to a bare '.star', so an absolute + # value gave the key two meanings -- and Path.__truediv__ discards the + # left side when the right is absolute, which pinned the manifest to + # whatever PROJECT_ROOT installed it. Moving or redeploying the install + # then left the app unable to find its own file. + 'star_file': dest.name, + } + return _write_starlark_manifest(manifest) def _starlark_virtual_plugins() -> list: """Installed Starlark apps, shaped like plugin entries. @@ -1757,14 +1803,15 @@ def _toggle_starlark_app(app_id: str, enabled: bool): # Only now is the in-memory copy allowed to disagree with disk. plugin.apps[safe_id].manifest['enabled'] = enabled else: - manifest = _read_starlark_manifest() - app_data = manifest.get('apps', {}).get(safe_id) - if not app_data: - return jsonify({'status': 'error', - 'message': f'Starlark app not found: {safe_id}'}), 404 - app_data['enabled'] = enabled - if not _write_starlark_manifest(manifest): - return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500 + with _starlark_manifest_lock(): + manifest = _read_starlark_manifest() + app_data = manifest.get('apps', {}).get(safe_id) + if not app_data: + return jsonify({'status': 'error', + 'message': f'Starlark app not found: {safe_id}'}), 404 + app_data['enabled'] = enabled + if not _write_starlark_manifest(manifest): + return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500 return jsonify({'status': 'success', 'message': f"Starlark app {'enabled' if enabled else 'disabled'}", diff --git a/web_interface/blueprints/api_v3/backup.py b/web_interface/blueprints/api_v3/backup.py index 722c68ed..f971f0e7 100644 --- a/web_interface/blueprints/api_v3/backup.py +++ b/web_interface/blueprints/api_v3/backup.py @@ -4,8 +4,9 @@ Routes decorate the shared `api_v3` Blueprint from ._common, so their endpoint names are unchanged by living here. """ from web_interface.blueprints.api_v3 import ( - PROJECT_ROOT, Path, _safe_backup_path, api_v3, datetime, json, jsonify, - logger, os, plugin_store_manager, request, tempfile, + PROJECT_ROOT, Path, _coerce_to_bool, _safe_backup_path, api_v3, + datetime, json, jsonify, logger, os, plugin_store_manager, request, + tempfile, ) import web_interface.blueprints.api_v3 as _pkg # Read through the module rather than bound by value: tests patch these @@ -111,13 +112,17 @@ def backup_restore(): 'status': 'error', 'message': 'Invalid options: expected a JSON object', }), 400 + # _coerce_to_bool (not bare bool()) because a request can send these + # as JSON strings: bool("false") is True in Python, so a caller who + # explicitly asked to skip secrets would have had them restored + # anyway. options = RestoreOptions( - restore_config=bool(opts_dict.get('restore_config', True)), - restore_secrets=bool(opts_dict.get('restore_secrets', True)), - restore_wifi=bool(opts_dict.get('restore_wifi', True)), - restore_fonts=bool(opts_dict.get('restore_fonts', True)), - restore_plugin_uploads=bool(opts_dict.get('restore_plugin_uploads', True)), - reinstall_plugins=bool(opts_dict.get('reinstall_plugins', True)), + restore_config=_coerce_to_bool(opts_dict.get('restore_config', True)), + restore_secrets=_coerce_to_bool(opts_dict.get('restore_secrets', True)), + restore_wifi=_coerce_to_bool(opts_dict.get('restore_wifi', True)), + restore_fonts=_coerce_to_bool(opts_dict.get('restore_fonts', True)), + restore_plugin_uploads=_coerce_to_bool(opts_dict.get('restore_plugin_uploads', True)), + reinstall_plugins=_coerce_to_bool(opts_dict.get('reinstall_plugins', True)), ) with tempfile.NamedTemporaryFile(suffix='.zip', delete=False) as tmp: tmp_path = tmp.name diff --git a/web_interface/blueprints/api_v3/config.py b/web_interface/blueprints/api_v3/config.py index b6cb5c01..d846d4fc 100644 --- a/web_interface/blueprints/api_v3/config.py +++ b/web_interface/blueprints/api_v3/config.py @@ -84,7 +84,7 @@ def save_schedule_config(): start_time = data.get('start_time', '07:00') end_time = data.get('end_time', '23:00') - # Validate _pkg.time formats + # Validate time formats is_valid, error_msg = _validate_time_format(start_time) if not is_valid: return error_response( @@ -148,12 +148,12 @@ def save_schedule_config(): else: end_time = '23:00' - # Validate _pkg.time formats + # Validate time formats is_valid, error_msg = _validate_time_format(start_time) if not is_valid: return error_response( ErrorCode.VALIDATION_ERROR, - f"Invalid start _pkg.time for {day}: {error_msg}", + f"Invalid start time for {day}: {error_msg}", status_code=400 ) @@ -161,7 +161,7 @@ def save_schedule_config(): if not is_valid: return error_response( ErrorCode.VALIDATION_ERROR, - f"Invalid end _pkg.time for {day}: {error_msg}", + f"Invalid end time for {day}: {error_msg}", status_code=400 ) @@ -313,7 +313,7 @@ def save_dim_schedule_config(): start_time = data.get('start_time', '20:00') end_time = data.get('end_time', '07:00') - # Validate _pkg.time formats + # Validate time formats is_valid, error_msg = _validate_time_format(start_time) if not is_valid: return error_response( @@ -365,12 +365,12 @@ def save_dim_schedule_config(): start_time = data.get(start_key) or '20:00' end_time = data.get(end_key) or '07:00' - # Validate _pkg.time formats + # Validate time formats is_valid, error_msg = _validate_time_format(start_time) if not is_valid: return error_response( ErrorCode.VALIDATION_ERROR, - f"Invalid start _pkg.time for {day}: {error_msg}", + f"Invalid start time for {day}: {error_msg}", status_code=400 ) @@ -378,7 +378,7 @@ def save_dim_schedule_config(): if not is_valid: return error_response( ErrorCode.VALIDATION_ERROR, - f"Invalid end _pkg.time for {day}: {error_msg}", + f"Invalid end time for {day}: {error_msg}", status_code=400 ) diff --git a/web_interface/blueprints/api_v3/display.py b/web_interface/blueprints/api_v3/display.py index 3f449bff..ac3efc0d 100644 --- a/web_interface/blueprints/api_v3/display.py +++ b/web_interface/blueprints/api_v3/display.py @@ -227,7 +227,7 @@ def start_on_demand_display(): # Stop the display service first to ensure clean state when we will restart it if service_was_running and start_service: - import _pkg.time as time_module + import time as time_module logger.debug("Stopping display service before starting on-demand mode") _stop_display_service() # Wait a brief moment for the service to fully stop diff --git a/web_interface/blueprints/api_v3/fonts.py b/web_interface/blueprints/api_v3/fonts.py index 897d04d5..ad966ae6 100644 --- a/web_interface/blueprints/api_v3/fonts.py +++ b/web_interface/blueprints/api_v3/fonts.py @@ -168,15 +168,30 @@ def upload_font(): if font_file.filename == '': return jsonify({'status': 'error', 'message': 'No file selected'}), 400 - # Validate filename + # Validate filename. validate_file_upload takes max_size_mb but only + # checks the filename/extension with it -- it never looks at the + # actual upload size, so the size limit below is enforced separately + # before the file is saved (same pattern as the .star upload above). + MAX_FONT_SIZE_MB = 10 is_valid, error_msg = validate_file_upload( font_file.filename, - max_size_mb=10, + max_size_mb=MAX_FONT_SIZE_MB, allowed_extensions=['.ttf', '.otf', '.bdf'] ) if not is_valid: return jsonify({'status': 'error', 'message': error_msg}), 400 + # Check file size (stated limit is MAX_FONT_SIZE_MB) + font_file.seek(0, 2) # Seek to end + file_size = font_file.tell() + font_file.seek(0) # Reset to beginning + max_font_size_bytes = MAX_FONT_SIZE_MB * 1024 * 1024 + if file_size > max_font_size_bytes: + return jsonify({ + 'status': 'error', + 'message': f'File too large (max {MAX_FONT_SIZE_MB}MB, got {file_size / 1024 / 1024:.1f}MB)' + }), 400 + font_family = request.form.get('font_family', '') if not font_family: diff --git a/web_interface/blueprints/api_v3/starlark.py b/web_interface/blueprints/api_v3/starlark.py index 4e812b10..8634fbbf 100644 --- a/web_interface/blueprints/api_v3/starlark.py +++ b/web_interface/blueprints/api_v3/starlark.py @@ -10,9 +10,10 @@ from web_interface.blueprints.api_v3 import ( _find_pixlet_binary, _install_star_file, _pixlet_editor_alive, _pixlet_editor_status, _read_pixlet_editor_state, _standalone_render_starlark_app, _starlark_github_token, - _validate_and_sanitize_app_id, _validate_starlark_app_path, - _validate_timing_value, api_v3, contextlib, describe_exception, json, - jsonify, logger, os, request, shutil, signal, subprocess, tempfile, + _starlark_manifest_lock, _validate_and_sanitize_app_id, + _validate_starlark_app_path, _validate_timing_value, api_v3, contextlib, + describe_exception, json, jsonify, logger, os, request, shutil, signal, + subprocess, tempfile, ) import web_interface.blueprints.api_v3 as _pkg # Read through the module rather than bound by value: tests patch these @@ -299,9 +300,10 @@ def uninstall_starlark_app(app_id): import shutil if app_dir.exists(): shutil.rmtree(app_dir) - manifest = _pkg._read_starlark_manifest() - manifest.get('apps', {}).pop(app_id, None) - success = _pkg._write_starlark_manifest(manifest) + with _starlark_manifest_lock(): + manifest = _pkg._read_starlark_manifest() + manifest.get('apps', {}).pop(app_id, None) + success = _pkg._write_starlark_manifest(manifest) if success: return jsonify({'status': 'success', 'message': f'App uninstalled: {app_id}'}) @@ -435,52 +437,70 @@ def update_starlark_app_config(app_id): else: return jsonify({'status': 'error', 'message': 'Failed to save configuration'}), 500 - # Standalone: update both config.json and manifest - manifest = _pkg._read_starlark_manifest() - app_data = manifest.get('apps', {}).get(app_id) - if not app_data: - return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404 + # Standalone: update both config.json and manifest. Both live under + # the manifest lock (serialized against every other standalone + # manifest read-modify-write -- see _starlark_manifest_lock), and if + # the manifest write fails after config.json was already written, + # config.json is rolled back so the two do not end up out of sync. + with _starlark_manifest_lock(): + manifest = _pkg._read_starlark_manifest() + app_data = manifest.get('apps', {}).get(app_id) + if not app_data: + return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404 - # Extract timing keys (they go in manifest, not config.json) - render_interval = data.pop('render_interval', None) - display_duration = data.pop('display_duration', None) + # Extract timing keys (they go in manifest, not config.json) + render_interval = data.pop('render_interval', None) + display_duration = data.pop('display_duration', None) - # Update manifest with timing values - if render_interval is not None: - app_data['render_interval'] = render_interval - if display_duration is not None: - app_data['display_duration'] = display_duration + # Update manifest with timing values + if render_interval is not None: + app_data['render_interval'] = render_interval + if display_duration is not None: + app_data['display_duration'] = display_duration - # Load current config from config.json. app_dir is the path - # _validate_starlark_app_path checked, not a fresh join. - config_file = app_dir / "config.json" - current_config = {} - if config_file.exists(): + # Load current config from config.json. app_dir is the path + # _validate_starlark_app_path checked, not a fresh join. + config_file = app_dir / "config.json" + existed_before = config_file.exists() + previous_config_bytes = None + current_config = {} + if existed_before: + try: + previous_config_bytes = config_file.read_bytes() + current_config = json.loads(previous_config_bytes) + except Exception as e: + logger.warning(f"Failed to load config for {app_id}: {e}") + + # Update config with new values (excluding timing keys) + current_config.update(data) + + # Write updated config to config.json try: - with open(config_file, 'r') as f: - current_config = json.load(f) + with open(config_file, 'w') as f: + json.dump(current_config, f, indent=2) except Exception as e: - logger.warning(f"Failed to load config for {app_id}: {e}") + logger.error(f"Failed to save config.json for {app_id}: {e}") + logger.exception("Failed to save Starlark configuration for %r", app_id) + return jsonify({'status': 'error', 'message': 'Failed to save configuration', + 'details': describe_exception(e)}), 500 - # Update config with new values (excluding timing keys) - current_config.update(data) + # Also update manifest for backward compatibility + app_data.setdefault('config', {}).update(data) - # Write updated config to config.json - try: - with open(config_file, 'w') as f: - json.dump(current_config, f, indent=2) - except Exception as e: - logger.error(f"Failed to save config.json for {app_id}: {e}") - logger.exception("Failed to save Starlark configuration for %r", app_id) - return jsonify({'status': 'error', 'message': 'Failed to save configuration', - 'details': describe_exception(e)}), 500 + if _pkg._write_starlark_manifest(manifest): + return jsonify({'status': 'success', 'message': 'Configuration updated', 'config': current_config}) - # Also update manifest for backward compatibility - app_data.setdefault('config', {}).update(data) - - if _pkg._write_starlark_manifest(manifest): - return jsonify({'status': 'success', 'message': 'Configuration updated', 'config': current_config}) - else: + # The manifest write failed after config.json was already + # written -- roll config.json back rather than leave the two + # disagreeing about what was saved. + try: + if existed_before and previous_config_bytes is not None: + config_file.write_bytes(previous_config_bytes) + elif not existed_before: + config_file.unlink(missing_ok=True) + except OSError: + logger.exception( + "Failed to roll back config.json for %r after manifest write failure", app_id) return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500 except Exception as e: @@ -508,19 +528,20 @@ def toggle_starlark_app(app_id): return jsonify({'status': 'success', 'message': f"App {'enabled' if enabled else 'disabled'}", 'enabled': enabled}) # Standalone: update manifest directly - manifest = _pkg._read_starlark_manifest() - app_data = manifest.get('apps', {}).get(app_id) - if not app_data: - return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404 + with _starlark_manifest_lock(): + manifest = _pkg._read_starlark_manifest() + app_data = manifest.get('apps', {}).get(app_id) + if not app_data: + return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404 - enabled = data.get('enabled') - if enabled is None: - enabled = not app_data.get('enabled', True) - app_data['enabled'] = enabled - if _pkg._write_starlark_manifest(manifest): - return jsonify({'status': 'success', 'message': f"App {'enabled' if enabled else 'disabled'}", 'enabled': enabled}) - else: - return jsonify({'status': 'error', 'message': 'Failed to save'}), 500 + enabled = data.get('enabled') + if enabled is None: + enabled = not app_data.get('enabled', True) + app_data['enabled'] = enabled + if _pkg._write_starlark_manifest(manifest): + return jsonify({'status': 'success', 'message': f"App {'enabled' if enabled else 'disabled'}", 'enabled': enabled}) + else: + return jsonify({'status': 'error', 'message': 'Failed to save'}), 500 except Exception as e: logger.exception("[Starlark] toggle_starlark_app failed") diff --git a/web_interface/blueprints/api_v3/wifi.py b/web_interface/blueprints/api_v3/wifi.py index a3b9c013..7827347e 100644 --- a/web_interface/blueprints/api_v3/wifi.py +++ b/web_interface/blueprints/api_v3/wifi.py @@ -8,6 +8,36 @@ from web_interface.blueprints.api_v3 import ( ) +def _parse_bool_ish(value, default=False): + """Coerce a JSON value that is supposed to be a boolean. + + A JSON boolean arrives as a real Python bool, but these routes are a + public HTTP contract and not every caller sends one. `bool(value)` gets + two common cases wrong: `bool("false")` is True (a non-empty string is + always truthy), and a plain int does not match an `is True` check + (`1 is True` is False, since `True` is a distinct singleton from the int + `1`) -- so a caller sending `{"enabled": 1}` was silently treated as + False. Recognizes a real bool, "true"/"false"/"1"/"0"/"yes"/"no" + case-insensitively, and int 1/0; anything else falls back to `default`. + """ + if isinstance(value, bool): + return value + if isinstance(value, str): + lowered = value.strip().lower() + if lowered in ('true', '1', 'yes'): + return True + if lowered in ('false', '0', 'no'): + return False + return default + if isinstance(value, int): + if value == 1: + return True + if value == 0: + return False + return default + return default + + # WiFi Management Endpoints @api_v3.route('/wifi/status', methods=['GET']) def get_wifi_status(): @@ -273,7 +303,7 @@ def set_auto_enable_ap_mode(): 'message': 'auto_enable_ap_mode is required' }), 400 - auto_enable = bool(data['auto_enable_ap_mode']) + auto_enable = _parse_bool_ish(data['auto_enable_ap_mode'], default=False) wifi_manager = WiFiManager() wifi_manager.config["auto_enable_ap_mode"] = auto_enable @@ -330,13 +360,12 @@ def set_wifi_radio(): 'message': 'enabled is required' }), 400 - # Parse defensively: bool("false") is True, so mirror the string-aware - # coercion used for `force` — the endpoint is a public contract, not just - # the shipped UI (which always sends real JSON booleans). - _enabled_raw = data['enabled'] - enabled = _enabled_raw is True or (isinstance(_enabled_raw, str) and _enabled_raw.lower() in ('true', '1', 'yes')) - _force_raw = data.get('force', False) - force = _force_raw is True or (isinstance(_force_raw, str) and _force_raw.lower() in ('true', '1', 'yes')) + # Parse defensively: bool("false") is True and a plain int never + # matches `is True`, so `_parse_bool_ish` handles bool, string and + # int 1/0 — the endpoint is a public contract, not just the shipped + # UI (which always sends real JSON booleans). + enabled = _parse_bool_ish(data['enabled'], default=False) + force = _parse_bool_ish(data.get('force', False), default=False) wifi_manager = WiFiManager() success, message, reason = wifi_manager.set_wifi_radio(enabled, force=force)