From 7c3e4b8a38e5990d755f169f80a6bc359dfe3ca6 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 10:35:27 +0000 Subject: [PATCH] fix(api-v3): address CodeRabbit findings on the blueprint split - _redact_credentials: blank scalar descendants of objects reached through a credential-owned list (e.g. tokens: [{"value": "secret"}]) regardless of field name -- the existing name-based walk only protected direct dict values under a credential key, not list items. - wifi.py: reject enabled/force/auto_enable_ap_mode values _parse_bool_ish can't recognize (400) instead of silently treating them as False, which could disable Wi-Fi or the radio itself. - Starlark manifest locking: lock a stable manifest.json.lock sidecar instead of manifest.json itself, in both the standalone route path (_starlark_manifest_lock) and the plugin path (StarlarkAppsPlugin._save_manifest / _update_manifest_safe). manifest.json is replaced by an atomic rename on every write, which swaps in a fresh inode; a lock held on the old inode does not exclude a second locker that opens the path afresh right after the rename and gets the new inode, so two writers could race despite each holding "a lock". A sidecar that no write ever touches always resolves to the same inode for every locker. Skipped as stale: the "serialize the complete manifest read-modify-write" finding at api_v3/__init__.py -- every standalone handler that calls _write_starlark_manifest is already wrapped in _starlark_manifest_lock() on this branch. Co-Authored-By: Claude Opus 5 --- plugin-repos/starlark-apps/manager.py | 21 ++++++--- test/test_api_v3_wifi_endpoints.py | 25 ++++++++++- test/test_config_main_redacts_secrets.py | 11 +++++ test/test_starlark_display_contract.py | 45 +++++++++++++++++++ .../test_starlark_pixlet_routes.py | 32 +++++++++++++ web_interface/blueprints/api_v3/__init__.py | 42 +++++++++++++---- web_interface/blueprints/api_v3/wifi.py | 41 +++++++++++++---- 7 files changed, 192 insertions(+), 25 deletions(-) diff --git a/plugin-repos/starlark-apps/manager.py b/plugin-repos/starlark-apps/manager.py index 4f370a85..999b98a7 100644 --- a/plugin-repos/starlark-apps/manager.py +++ b/plugin-repos/starlark-apps/manager.py @@ -220,6 +220,8 @@ class StarlarkAppsPlugin(BasePlugin): # App storage self.apps_dir = self._get_apps_directory() self.manifest_file = self.apps_dir / "manifest.json" + # A dedicated, never-replaced file to flock -- see _update_manifest_safe. + self.manifest_lock_file = self.apps_dir / "manifest.json.lock" self.apps: Dict[str, StarlarkApp] = {} # Display state @@ -564,7 +566,8 @@ class StarlarkAppsPlugin(BasePlugin): def _save_manifest(self, manifest: Dict[str, Any]) -> bool: """ Save apps manifest to file with file locking to prevent race conditions. - Acquires exclusive lock on manifest file before writing to prevent concurrent modifications. + Acquires exclusive lock on the manifest lock sidecar before writing to + prevent concurrent modifications. """ temp_file = None lock_fd = None @@ -572,9 +575,14 @@ class StarlarkAppsPlugin(BasePlugin): # Create parent directory if needed self.manifest_file.parent.mkdir(parents=True, exist_ok=True) - # Open manifest file for locking (create if doesn't exist, don't truncate) - # Use os.open with O_CREAT | O_RDWR to create if missing, but don't truncate - lock_fd = os.open(str(self.manifest_file), os.O_CREAT | os.O_RDWR, 0o644) + # Lock the sidecar file, not manifest_file itself: manifest_file is + # replaced by an atomic rename below, which swaps in a fresh inode + # a second locker's fresh os.open() would pick up unguarded. The + # sidecar is never written to or renamed over, so it always + # resolves to the same inode for every locker (see + # _update_manifest_safe and web_interface's _starlark_manifest_lock, + # which must lock this same file for the guarantee to hold). + lock_fd = os.open(str(self.manifest_lock_file), os.O_CREAT | os.O_RDWR, 0o644) # Acquire exclusive lock on manifest file BEFORE creating temp file # This serializes all writers and prevents concurrent races @@ -630,8 +638,9 @@ class StarlarkAppsPlugin(BasePlugin): # Create parent directory if needed self.manifest_file.parent.mkdir(parents=True, exist_ok=True) - # Open manifest file for locking (create if doesn't exist, don't truncate) - lock_fd = os.open(str(self.manifest_file), os.O_CREAT | os.O_RDWR, 0o644) + # Lock the sidecar file, not manifest_file itself -- see the + # comment in _save_manifest for why. + lock_fd = os.open(str(self.manifest_lock_file), os.O_CREAT | os.O_RDWR, 0o644) # Acquire exclusive lock for entire read-modify-write cycle fcntl.flock(lock_fd, fcntl.LOCK_EX) diff --git a/test/test_api_v3_wifi_endpoints.py b/test/test_api_v3_wifi_endpoints.py index 0b4a81ea..344aef8b 100644 --- a/test/test_api_v3_wifi_endpoints.py +++ b/test/test_api_v3_wifi_endpoints.py @@ -195,7 +195,7 @@ class TestRadio: @pytest.mark.parametrize("raw,expected", [ (True, True), ("true", True), ("1", True), ("yes", True), - (False, False), ("false", False), ("off", False), (0, False), + (False, False), ("false", False), ("no", False), (0, False), ]) def test_enabled_coercion_is_string_aware( self, api_v3_client, wifi_manager, raw, expected): @@ -228,6 +228,22 @@ class TestRadio: wifi_manager.set_wifi_radio.side_effect = RuntimeError("boom") assert api_v3_client.post(self.URL, json={"enabled": True}).status_code == 500 + def test_unrecognized_enabled_value_is_rejected_not_treated_as_false( + self, api_v3_client, wifi_manager): + # An invalid `enabled` used to silently fall back to False, which + # can disconnect Wi-Fi (or, with force=true, drop the caller's own + # connection to this interface) even though nothing asked for that. + response = api_v3_client.post(self.URL, json={"enabled": "typo"}) + assert response.status_code == 400 + wifi_manager.set_wifi_radio.assert_not_called() + + def test_unrecognized_force_value_is_rejected_not_treated_as_false( + self, api_v3_client, wifi_manager): + response = api_v3_client.post( + self.URL, json={"enabled": False, "force": "typo"}) + assert response.status_code == 400 + wifi_manager.set_wifi_radio.assert_not_called() + class TestAutoEnableApMode: URL = "/api/v3/wifi/ap/auto-enable" @@ -259,6 +275,13 @@ class TestAutoEnableApMode: api_v3_client.post(self.URL, json={"auto_enable_ap_mode": "false"}) assert wifi_manager.config["auto_enable_ap_mode"] is False + def test_unrecognized_value_is_rejected_not_treated_as_false( + self, api_v3_client, wifi_manager): + wifi_manager.config = {} + response = api_v3_client.post(self.URL, json={"auto_enable_ap_mode": "typo"}) + assert response.status_code == 400 + assert "auto_enable_ap_mode" not in wifi_manager.config + class TestRadioEnabledAndForceAcceptIntegers: """`{"enabled": 1}` / `{"enabled": 0}` used to be mishandled: the old diff --git a/test/test_config_main_redacts_secrets.py b/test/test_config_main_redacts_secrets.py index 191f8d20..d8f94993 100644 --- a/test/test_config_main_redacts_secrets.py +++ b/test/test_config_main_redacts_secrets.py @@ -125,6 +125,17 @@ def test_a_list_nested_inside_a_credential_named_container_is_also_blanked(): assert out["auth"]["note"] == "keep" +def test_objects_inside_a_credential_named_list_are_blanked_regardless_of_field_names(): + """A list item has no field name of its own to test against + `_CREDENTIAL_NAME_PARTS`, so an object reached through a credential-owned + list must be blanked outright rather than walked by field name -- unlike + the dict-directly-under-a-credential-key case in the test above. + """ + config = {"tokens": [{"value": "secret", "kind": "bearer"}]} + out = _redact_credentials(config) + assert out["tokens"] == [{"value": "", "kind": ""}] + + 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 diff --git a/test/test_starlark_display_contract.py b/test/test_starlark_display_contract.py index be05393d..b8713c24 100644 --- a/test/test_starlark_display_contract.py +++ b/test/test_starlark_display_contract.py @@ -6,6 +6,7 @@ frame for the whole display_duration instead of rotating on -- the same class of defect as a display() that returns None. """ +import json import sys import types from pathlib import Path @@ -197,6 +198,50 @@ class TestInstalledAppsTakeTurns: assert p.display(force_clear=True) is False +class TestManifestLockSidecarIsStable: + """_save_manifest and _update_manifest_safe must lock a sidecar file that + a manifest write never replaces. manifest.json itself is swapped for a + fresh inode by every atomic write (temp file + rename); a lock taken on + manifest.json directly would not exclude a second locker whose fresh + os.open() lands on that new inode right after a write, so two writers + could still race each other despite each believing it held the lock. + web_interface._starlark_manifest_lock (web_interface/blueprints/api_v3) + must lock this same sidecar name for that guarantee to hold across both + the plugin-loaded and standalone paths. + """ + + def _plugin_with_manifest_dir(self, manager_module, tmp_path): + p = _plugin(manager_module) + p.manifest_file = tmp_path / "manifest.json" + p.manifest_lock_file = tmp_path / "manifest.json.lock" + return p + + def test_the_lock_sidecar_is_not_the_manifest_file(self, manager_module, tmp_path): + p = self._plugin_with_manifest_dir(manager_module, tmp_path) + assert p.manifest_lock_file != p.manifest_file + assert p.manifest_lock_file.name == "manifest.json.lock" + + def test_the_locked_files_inode_survives_repeated_writes(self, manager_module, tmp_path): + p = self._plugin_with_manifest_dir(manager_module, tmp_path) + + assert p._save_manifest({"apps": {}}) + lock_ino_before = p.manifest_lock_file.stat().st_ino + + for app_id in ("one", "two", "three"): + def _update(manifest, app_id=app_id): + manifest.setdefault("apps", {})[app_id] = {"enabled": True} + assert p._update_manifest_safe(_update) + + lock_ino_after = p.manifest_lock_file.stat().st_ino + assert lock_ino_after == lock_ino_before, ( + "the locked file's inode changed across writes -- a locker that " + "opened it before this write and one that opens it after would " + "no longer contend for the same lock") + + manifest = json.loads(p.manifest_file.read_text()) + assert set(manifest["apps"]) == {"one", "two", "three"} + + class TestAnimationsRunAtFrameRate: def test_the_plugin_asks_for_the_high_fps_loop(self, manager_module): """The controller reads this attribute; a multi-frame app is otherwise diff --git a/test/web_interface/test_starlark_pixlet_routes.py b/test/web_interface/test_starlark_pixlet_routes.py index e1c09a86..2d42734b 100644 --- a/test/web_interface/test_starlark_pixlet_routes.py +++ b/test/web_interface/test_starlark_pixlet_routes.py @@ -413,9 +413,41 @@ class TestManifestLockPreventsLostUpdates: apps_dir.mkdir() monkeypatch.setattr(module, '_STARLARK_APPS_DIR', apps_dir) monkeypatch.setattr(module, '_STARLARK_MANIFEST_FILE', apps_dir / 'manifest.json') + monkeypatch.setattr(module, '_STARLARK_MANIFEST_LOCK_FILE', apps_dir / 'manifest.json.lock') module._write_starlark_manifest({'apps': {}}) return apps_dir + def test_the_locked_file_survives_a_manifest_write(self, starlark_dir): + """_write_starlark_manifest replaces manifest.json with a fresh inode + on every write (temp file + rename). If the lock were taken on that + same file, a second locker's fresh os.open() right after the rename + would land on the new inode -- unguarded, because only the old, + now-orphaned inode was ever locked -- and two writers could race + despite each believing it "held the lock" (see the docstring on + _starlark_manifest_lock). Locking a sidecar path that no write ever + touches or renames over closes that: the inode identity of what gets + locked must not change across writes. + """ + import os + + from web_interface.blueprints import api_v3 as module + + with module._starlark_manifest_lock(): + manifest = module._read_starlark_manifest() + lock_ino_before = os.stat(module._STARLARK_MANIFEST_LOCK_FILE).st_ino + + for app_id in ('one', 'two', 'three'): + with module._starlark_manifest_lock(): + manifest = module._read_starlark_manifest() + manifest.setdefault('apps', {})[app_id] = {'enabled': True} + assert module._write_starlark_manifest(manifest) + + lock_ino_after = os.stat(module._STARLARK_MANIFEST_LOCK_FILE).st_ino + assert lock_ino_after == lock_ino_before, ( + "the locked file's inode changed across writes -- a locker that " + "opened it before this write and one that opens it after would " + "no longer contend for the same lock") + def test_two_concurrent_updates_are_both_kept(self, starlark_dir): import threading import time as _time diff --git a/web_interface/blueprints/api_v3/__init__.py b/web_interface/blueprints/api_v3/__init__.py index d51d5609..050412f7 100644 --- a/web_interface/blueprints/api_v3/__init__.py +++ b/web_interface/blueprints/api_v3/__init__.py @@ -302,24 +302,36 @@ def _redact_credentials(value): if isinstance(value, list): return [_redact_credentials(item) for item in value] return value +def _blank_all_scalars(value): + """Blank every scalar reached from `value`, at any depth. + + Unlike `_blank_credential_value`, this never delegates back to + `_redact_credentials`'s name-based walk: an object reached through a + credential-owned list (e.g. `tokens: [{"value": "secret"}]`) has no + field name of its own to test, so every scalar inside it is blanked + regardless of what its keys are called. + """ + if isinstance(value, dict): + return {k: _blank_all_scalars(v) for k, v in value.items()} + if isinstance(value, list): + return [_blank_all_scalars(item) for item in value] + return "" 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. + test_a_credential_shaped_container_is_still_walked). A list has no field + names to test for its items, though, so every scalar reached through it + is blanked outright: a bare list of secrets (`tokens: ["a", "b"]`) and a + list of credential-shaped objects (`tokens: [{"value": "secret"}]`) are + both blanked at any depth via `_blank_all_scalars`. """ if isinstance(value, dict): return _redact_credentials(value) if isinstance(value, list): - return [_blank_credential_value(item) for item in value] + return [_blank_all_scalars(item) for item in value] return "" def _validate_time_format(time_str): """Validate time format is HH:MM""" @@ -1303,6 +1315,8 @@ def _safe_backup_path(filename: str) -> Path: return path _STARLARK_APPS_DIR = PROJECT_ROOT / 'starlark-apps' _STARLARK_MANIFEST_FILE = _STARLARK_APPS_DIR / 'manifest.json' +# A dedicated, never-replaced file to flock -- see _starlark_manifest_lock. +_STARLARK_MANIFEST_LOCK_FILE = _STARLARK_APPS_DIR / 'manifest.json.lock' def _get_starlark_plugin() -> Optional[Any]: """Get the starlark-apps plugin instance, or None.""" if not api_v3.plugin_manager: @@ -1353,11 +1367,21 @@ def _starlark_manifest_lock(): 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. + Locks _STARLARK_MANIFEST_LOCK_FILE, a sidecar that is never written to or + renamed over -- not manifest.json itself. manifest.json is replaced by an + atomic rename on every write (here and in the plugin), which swaps in a + fresh inode; a lock held on the old inode does not exclude a second locker + that opens the path afresh right after the rename and gets the new inode, + so two writers could still race each other despite both "holding a lock". + A stable sidecar path always resolves to the same inode, so every locker + -- standalone or plugin-owned -- contends for the same lock. The plugin + must lock this same sidecar file for that guarantee to hold across both. + 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) + lock_fd = os.open(str(_STARLARK_MANIFEST_LOCK_FILE), os.O_CREAT | os.O_RDWR, 0o644) try: fcntl.flock(lock_fd, fcntl.LOCK_EX) try: diff --git a/web_interface/blueprints/api_v3/wifi.py b/web_interface/blueprints/api_v3/wifi.py index 7827347e..065a4b63 100644 --- a/web_interface/blueprints/api_v3/wifi.py +++ b/web_interface/blueprints/api_v3/wifi.py @@ -8,7 +8,7 @@ from web_interface.blueprints.api_v3 import ( ) -def _parse_bool_ish(value, default=False): +def _parse_bool_ish(value): """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 @@ -18,7 +18,13 @@ def _parse_bool_ish(value, default=False): (`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`. + case-insensitively, and int 1/0. + + Returns None for anything else, rather than guessing. A supplied-but- + unrecognized value (e.g. a typo) used to silently become False here, + which for `enabled` on the radio route could disconnect Wi-Fi the caller + never asked to turn off -- callers must treat None as a validation + error, not a real False. """ if isinstance(value, bool): return value @@ -28,14 +34,14 @@ def _parse_bool_ish(value, default=False): return True if lowered in ('false', '0', 'no'): return False - return default + return None if isinstance(value, int): if value == 1: return True if value == 0: return False - return default - return default + return None + return None # WiFi Management Endpoints @@ -303,7 +309,12 @@ def set_auto_enable_ap_mode(): 'message': 'auto_enable_ap_mode is required' }), 400 - auto_enable = _parse_bool_ish(data['auto_enable_ap_mode'], default=False) + auto_enable = _parse_bool_ish(data['auto_enable_ap_mode']) + if auto_enable is None: + return jsonify({ + 'status': 'error', + 'message': 'auto_enable_ap_mode must be a boolean' + }), 400 wifi_manager = WiFiManager() wifi_manager.config["auto_enable_ap_mode"] = auto_enable @@ -363,9 +374,21 @@ def set_wifi_radio(): # 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) + # UI (which always sends real JSON booleans). An unrecognized value + # must be rejected, not silently disable the radio: this is the + # route that can drop the caller's own connection to this interface. + enabled = _parse_bool_ish(data['enabled']) + if enabled is None: + return jsonify({ + 'status': 'error', + 'message': 'enabled must be a boolean' + }), 400 + force = _parse_bool_ish(data.get('force', False)) + if force is None: + return jsonify({ + 'status': 'error', + 'message': 'force must be a boolean' + }), 400 wifi_manager = WiFiManager() success, message, reason = wifi_manager.set_wifi_radio(enabled, force=force)