diff --git a/test/web_interface/test_starlark_pixlet_routes.py b/test/web_interface/test_starlark_pixlet_routes.py index 12697975..24ed1f2f 100644 --- a/test/web_interface/test_starlark_pixlet_routes.py +++ b/test/web_interface/test_starlark_pixlet_routes.py @@ -15,6 +15,11 @@ reads, so a future rewrite of this file cannot silently drop them again. """ import json +import os +import sys +import tempfile +import threading +import types from unittest.mock import MagicMock, patch import pytest @@ -28,6 +33,24 @@ def client(): yield c +@pytest.fixture +def offline_repository(): + """Stand in for TronbyteRepository so the store routes stay offline. + + The browse and categories routes call list_all_apps_cached(), which on a + cold server-side cache fetches the app index from GitHub. That would make + the suite slow, network-dependent and rate-limitable -- and because these + tests only assert "not a 404", a rate-limited 500 would pass and hide it. + """ + repo = MagicMock() + repo.list_all_apps_cached.return_value = { + 'apps': [], 'categories': [], 'authors': [], 'count': 0, 'cached': True, + } + with patch('web_interface.blueprints.api_v3._get_tronbyte_repository_class', + return_value=lambda *a, **kw: repo): + yield repo + + class TestRoutesAreRegistered: """The failure was a missing route, so check the URL map directly. @@ -133,12 +156,12 @@ class TestTheAppStoreFlow: an empty store. """ - def test_browse_does_not_404(self, client): + def test_browse_does_not_404(self, client, offline_repository): resp = client.get('/api/v3/starlark/repository/browse') assert resp.status_code != 404, "the store cannot list anything" assert resp.get_json().get('message') != 'Resource not found' - def test_categories_does_not_404(self, client): + def test_categories_does_not_404(self, client, offline_repository): resp = client.get('/api/v3/starlark/repository/categories') assert resp.status_code != 404 assert resp.get_json().get('message') != 'Resource not found' @@ -269,4 +292,216 @@ class TestInstalledAppsAppearWithTheOtherPlugins: resp = client.post('/api/v3/plugins/toggle', json={'plugin_id': 'starlark:../../etc/passwd', 'enabled': True}) assert resp.status_code == 400 - assert 'invalid characters' in resp.get_json()['message'] + assert 'traversal' in resp.get_json()['message'] + + def test_an_app_id_the_listing_published_can_be_toggled(self, client): + """The id here is the one _starlark_virtual_plugins publishes. + + It used to be re-slugified on the way in -- lowercased, with every + character outside [a-z0-9_] replaced -- so 'My-App' was listed as + 'starlark:My-App' and looked up as 'my_app', and toggling an app the + page had just drawn answered 404. + """ + manifest = {'apps': {'My-App': {'name': 'My App', 'enabled': False}}} + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \ + patch('web_interface.blueprints.api_v3._read_starlark_manifest', + return_value=manifest), \ + patch('web_interface.blueprints.api_v3._write_starlark_manifest', + return_value=True) as write: + resp = client.post('/api/v3/plugins/toggle', + json={'plugin_id': 'starlark:My-App', 'enabled': True}) + assert resp.status_code == 200, resp.get_json() + assert write.called, "the toggle never reached the manifest" + assert manifest['apps']['My-App']['enabled'] is True + + def test_a_failed_manifest_write_is_not_reported_as_success(self, client): + """The toggle answered 200 while the change was never persisted.""" + manifest = {'apps': {'demo': {'name': 'Demo', 'enabled': False}}} + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=None), \ + patch('web_interface.blueprints.api_v3._read_starlark_manifest', + return_value=manifest), \ + patch('web_interface.blueprints.api_v3._write_starlark_manifest', + return_value=False): + resp = client.post('/api/v3/plugins/toggle', + json={'plugin_id': 'starlark:demo', 'enabled': True}) + assert resp.status_code == 500 + + def test_a_loaded_app_is_not_flipped_when_the_manifest_write_fails(self, client): + """Persist first, then update memory -- otherwise the UI shows a + toggle that silently reverts on the next restart.""" + app = MagicMock() + app.manifest = {'enabled': False} + plugin = MagicMock() + plugin.apps = {'demo': app} + plugin._update_manifest_safe.return_value = False + + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin): + resp = client.post('/api/v3/plugins/toggle', + json={'plugin_id': 'starlark:demo', 'enabled': True}) + assert resp.status_code == 500 + assert app.manifest['enabled'] is False, "in-memory state changed without being saved" + + +class TestTheManifestSurvivesConcurrentWriters: + """Flask serves requests concurrently and five routes write this file. + + The atomic-write pattern used one fixed temp name, `manifest.tmp`, shared + by every writer: two of them opened it, interleaved their json.dump output, + and both renamed. The rename is atomic; the content it published was the + mixture, which the next read could not parse. + """ + + @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') + return apps_dir + + def test_each_writer_gets_its_own_temp_file(self, starlark_dir): + from web_interface.blueprints import api_v3 as module + + seen = [] + real_mkstemp = tempfile.mkstemp + + def recording_mkstemp(*args, **kwargs): + fd, name = real_mkstemp(*args, **kwargs) + seen.append(name) + return fd, name + + with patch.object(module.tempfile, 'mkstemp', side_effect=recording_mkstemp): + for i in range(5): + assert module._write_starlark_manifest({'apps': {f'app{i}': {}}}) + + assert len(set(seen)) == 5, f"writers shared a temp file: {seen}" + + def test_concurrent_writes_leave_readable_json(self, starlark_dir): + from web_interface.blueprints import api_v3 as module + + manifests = [{'apps': {f'app{i}': {'name': 'x' * 400}}} for i in range(8)] + errors = [] + + def write(m): + try: + module._write_starlark_manifest(m) + except Exception as exc: # noqa: BLE001 - surfaced by the assert below + errors.append(exc) + + threads = [threading.Thread(target=write, args=(m,)) for m in manifests] + for t in threads: + t.start() + for t in threads: + t.join() + + assert not errors + loaded = json.loads((starlark_dir / 'manifest.json').read_text()) + assert loaded in manifests, "the published manifest was a mix of two writers" + + def test_no_temp_files_are_left_behind(self, starlark_dir): + from web_interface.blueprints import api_v3 as module + module._write_starlark_manifest({'apps': {}}) + assert list(starlark_dir.glob('*.tmp')) == [] + + def test_the_installed_star_file_is_recorded_by_name(self, starlark_dir, tmp_path): + """Readers join this to the app's own directory and default to a bare + '.star'. An absolute value gave the key two meanings and, since + Path.__truediv__ discards the left side when the right is absolute, + pinned the manifest to the PROJECT_ROOT it was installed under.""" + from web_interface.blueprints import api_v3 as module + + source = tmp_path / "source.star" + source.write_text("# app") + with patch.object(module, '_get_pixlet_renderer_class', + side_effect=ImportError("no pixlet here")): + assert module._install_star_file('demo', str(source), {'name': 'Demo'}) + + entry = json.loads((starlark_dir / 'manifest.json').read_text())['apps']['demo'] + assert entry['star_file'] == 'demo.star' + assert not os.path.isabs(entry['star_file']) + + +class TestATransientImportFailureIsNotPermanent: + """Both importers insert into sys.modules before executing the module. + + That order is required -- a module has to be findable while it runs -- but + a failure left the half-initialised object cached, so every later call took + the cache branch and raised AttributeError on the missing class instead of + retrying. One transient failure disabled the repository or the renderer for + the life of the process. + """ + + @pytest.mark.parametrize("getter,key", [ + ('_get_tronbyte_repository_class', 'tronbyte_repository'), + ('_get_pixlet_renderer_class', 'pixlet_renderer'), + ]) + def test_a_failed_import_leaves_no_entry_behind(self, getter, key): + from web_interface.blueprints import api_v3 as module + + original = sys.modules.pop(key, None) + try: + with patch('importlib.util.module_from_spec') as from_spec: + from_spec.return_value = types.ModuleType(key) + with patch('importlib.util.spec_from_file_location') as spec_from: + spec = MagicMock() + spec.loader.exec_module.side_effect = RuntimeError("network down") + spec_from.return_value = spec + + with pytest.raises(RuntimeError): + getattr(module, getter)() + + assert key not in sys.modules, "a half-initialised module stayed cached" + finally: + if original is not None: + sys.modules[key] = original + else: + sys.modules.pop(key, None) + + +class TestConfigIsNotAppliedUntilItIsSaved: + """save_config() returning False answers 500, but the loaded app kept the + new values -- so GET config reported settings that were never written and + the plugin rendered with them until a restart silently reverted them.""" + + def _plugin_with_app(self, save_ok): + app = MagicMock() + app.config = {'city': 'Philadelphia'} + app.manifest = {'render_interval': 300, 'display_duration': 15} + app.save_config.return_value = save_ok + plugin = MagicMock() + plugin.apps = {'demo': app} + plugin._update_manifest_safe.return_value = True + return plugin, app + + def test_a_failed_save_leaves_the_config_untouched(self, client): + plugin, app = self._plugin_with_app(save_ok=False) + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin): + resp = client.put('/api/v3/starlark/apps/demo/config', + json={'city': 'Pittsburgh'}) + assert resp.status_code == 500 + assert app.config == {'city': 'Philadelphia'} + + def test_a_failed_save_leaves_the_timing_untouched(self, client): + plugin, app = self._plugin_with_app(save_ok=False) + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin): + resp = client.put('/api/v3/starlark/apps/demo/config', + json={'render_interval': 60}) + assert resp.status_code == 500 + assert app.manifest['render_interval'] == 300 + + def test_a_failed_save_does_not_re_render_with_values_it_did_not_keep(self, client): + plugin, _ = self._plugin_with_app(save_ok=False) + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin): + client.put('/api/v3/starlark/apps/demo/config', json={'city': 'Pittsburgh'}) + plugin._render_app.assert_not_called() + + def test_a_successful_save_still_applies(self, client): + plugin, app = self._plugin_with_app(save_ok=True) + with patch('web_interface.blueprints.api_v3._get_starlark_plugin', return_value=plugin): + resp = client.put('/api/v3/starlark/apps/demo/config', + json={'city': 'Pittsburgh', 'render_interval': 60}) + assert resp.status_code == 200 + assert app.config['city'] == 'Pittsburgh' + assert app.manifest['render_interval'] == 60 + plugin._render_app.assert_called_once() diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 06340a0c..711406c9 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -8833,8 +8833,21 @@ def _get_tronbyte_repository_class() -> Type[Any]: if module is None: raise ImportError("Failed to create module from spec for tronbyte_repository") + # The entry has to come out again if execution fails. It is inserted + # first because a module has to be in sys.modules before it runs (its own + # imports can reach back for it), but leaving a half-initialised module + # cached means every later call takes the branch above and raises + # AttributeError on the missing class instead of retrying -- one transient + # failure would disable the repository for the life of the process. + # + # The key stays unprefixed to match _get_pixlet_renderer_class, where the + # short name is load-bearing (see the note there). sys.modules["tronbyte_repository"] = module - spec.loader.exec_module(module) + try: + spec.loader.exec_module(module) + except Exception: + sys.modules.pop("tronbyte_repository", None) + raise return module.TronbyteRepository def _get_pixlet_renderer_class() -> Type[Any]: @@ -8858,8 +8871,19 @@ def _get_pixlet_renderer_class() -> Type[Any]: if module is None: raise ImportError("Failed to create module from spec for pixlet_renderer") + # Uncache on failure, as in _get_tronbyte_repository_class, so a transient + # import error does not disable rendering for the rest of the process. + # + # The short key is load-bearing here: the plugin's own manager.py does + # `from pixlet_renderer import PixletRenderer`, and this key is what makes + # both loaders share one module object. Prefixing it would quietly produce + # a second copy of the class, so isinstance across the two would fail. sys.modules["pixlet_renderer"] = module - spec.loader.exec_module(module) + try: + spec.loader.exec_module(module) + except Exception: + sys.modules.pop("pixlet_renderer", None) + raise return module.PixletRenderer def _validate_and_sanitize_app_id(app_id: Optional[str], fallback_source: Optional[str] = None) -> Tuple[Optional[str], Optional[str]]: @@ -9032,9 +9056,20 @@ def _write_starlark_manifest(manifest: Dict[str, Any]) -> bool: try: _STARLARK_APPS_DIR.mkdir(parents=True, exist_ok=True) - # Atomic write pattern: write to temp file, then rename - temp_file = _STARLARK_MANIFEST_FILE.with_suffix('.tmp') - with open(temp_file, 'w') as f: + # Atomic write pattern: write to temp file, then rename. + # + # The temp name has to be unique per writer, not a fixed + # manifest.tmp. Flask serves requests concurrently and this is called + # from the upload, uninstall, config, toggle and plugin-toggle routes, + # so two writers shared one file: both wrote into it, their json.dump + # output interleaved, and both renamed. The rename is still atomic -- + # what it publishes is the mixture, which the next read cannot parse. + # mkstemp in the destination directory gives each writer its own file + # while keeping the rename on one filesystem. + fd, temp_name = tempfile.mkstemp( + dir=str(_STARLARK_APPS_DIR), prefix='.manifest-', suffix='.tmp') + temp_file = Path(temp_name) + with os.fdopen(fd, 'w') as f: json.dump(manifest, f, indent=2) f.flush() os.fsync(f.fileno()) # Ensure data is written to disk @@ -9112,7 +9147,13 @@ def _install_star_file(app_id: str, star_file_path: str, metadata: Dict[str, Any 'render_interval': metadata.get('render_interval', 300), 'display_duration': metadata.get('display_duration', 15), 'config': metadata.get('config', {}), - 'star_file': str(dest), + # The filename, not the full path. Readers join this to the app's own + # directory and default 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 was at install time. Moving or redeploying the + # install then left the app unable to find its own file. + 'star_file': dest.name, } return _write_starlark_manifest(manifest) @@ -9414,6 +9455,15 @@ def update_starlark_app_config(app_id): render_interval = data.pop('render_interval', None) display_duration = data.pop('display_duration', None) + # Snapshot before mutating, so a failed save leaves nothing + # behind. save_config() returning False answers 500, but the + # loaded app kept the new values: the config endpoint then + # reported configuration that was never written, and the plugin + # went on rendering with it until the next restart silently put + # the old values back. + previous_config = dict(app.config) + previous_manifest = dict(app.manifest) + # Update config with non-timing fields only app.config.update(data) @@ -9425,27 +9475,32 @@ def update_starlark_app_config(app_id): if display_duration is not None: app.manifest['display_duration'] = display_duration timing_changed = True - if app.save_config(): - # Persist manifest if timing changed (same pattern as toggle endpoint) - if timing_changed: - try: - # Use safe manifest update to prevent race conditions - timing_updates = {} - if render_interval is not None: - timing_updates['render_interval'] = render_interval - if display_duration is not None: - timing_updates['display_duration'] = display_duration - - def update_fn(manifest): - manifest['apps'][app_id].update(timing_updates) - starlark_plugin._update_manifest_safe(update_fn) - except Exception as e: - logger.warning(f"Failed to persist timing to manifest for {app_id}: {e}") - starlark_plugin._render_app(app, force=True) - return jsonify({'status': 'success', 'message': 'Configuration updated', 'config': app.config}) - else: + if not app.save_config(): + app.config.clear() + app.config.update(previous_config) + app.manifest.clear() + app.manifest.update(previous_manifest) return jsonify({'status': 'error', 'message': 'Failed to save configuration'}), 500 + # Persist manifest if timing changed (same pattern as toggle endpoint) + if timing_changed: + try: + # Use safe manifest update to prevent race conditions + timing_updates = {} + if render_interval is not None: + timing_updates['render_interval'] = render_interval + if display_duration is not None: + timing_updates['display_duration'] = display_duration + + def update_fn(manifest): + manifest['apps'][app_id].update(timing_updates) + if not starlark_plugin._update_manifest_safe(update_fn): + logger.warning("Timing for %s was not persisted to the manifest", app_id) + except Exception as e: + logger.warning(f"Failed to persist timing to manifest for {app_id}: {e}") + starlark_plugin._render_app(app, force=True) + return jsonify({'status': 'success', 'message': 'Configuration updated', 'config': app.config}) + # Standalone: update both config.json and manifest manifest = _read_starlark_manifest() app_data = manifest.get('apps', {}).get(app_id) @@ -9753,18 +9808,36 @@ def _starlark_virtual_plugins() -> list: def _toggle_starlark_app(app_id: str, enabled: bool): """Enable or disable one Starlark app, loaded or not.""" - safe_id, err = _validate_and_sanitize_app_id(app_id) + # Check for traversal but keep the key as it was listed. + # _validate_and_sanitize_app_id lowercases and rewrites every character + # outside [a-z0-9_], while _starlark_virtual_plugins publishes the raw + # manifest key -- so an app stored as 'My-App' was offered to the UI as + # 'starlark:My-App' and looked up here as 'my_app', and toggling it + # answered 404 for an app the page had just drawn. Keys written by + # _install_star_file are already sanitised; ones written by the plugin or + # edited by hand are not. + _, err = _validate_starlark_app_path(app_id) if err: - return jsonify({'status': 'error', 'message': f'Invalid app_id: {err}'}), 400 + # err already names app_id; do not prefix it a second time. + return jsonify({'status': 'error', 'message': err}), 400 + safe_id = app_id plugin = _get_starlark_plugin() if plugin is not None and safe_id in getattr(plugin, 'apps', {}): - plugin.apps[safe_id].manifest['enabled'] = enabled - def _update(manifest): - manifest['apps'][safe_id]['enabled'] = enabled + # setdefault rather than indexing: the app is loaded, but the + # on-disk entry may not exist yet, and _update_manifest_safe does + # not catch KeyError -- it would escape as a 500 instead of the + # error this returns. + manifest.setdefault('apps', {}).setdefault(safe_id, {})['enabled'] = enabled - plugin._update_manifest_safe(_update) + # Persist first, then update what is loaded. The other order reported + # success and left the in-memory app disagreeing with the manifest + # whenever the write failed, so the UI showed a toggle that reverted + # itself on the next restart. + if not plugin._update_manifest_safe(_update): + return jsonify({'status': 'error', 'message': 'Failed to save manifest'}), 500 + plugin.apps[safe_id].manifest['enabled'] = enabled else: manifest = _read_starlark_manifest() app_data = manifest.get('apps', {}).get(safe_id)