From 6a60a57421bc3902dcd566216f87bcb5e86a415b Mon Sep 17 00:00:00 2001 From: Chuck Date: Thu, 19 Feb 2026 19:37:09 -0500 Subject: [PATCH] fix(starlark): security and race condition improvements Security fixes: - Add path traversal validation for output_path in download_star_file - Remove XSS-vulnerable inline onclick handlers, use delegated events - Add type hints to helper functions for better type safety Race condition fixes: - Lock manifest file BEFORE creating temp file in _save_manifest - Hold exclusive lock for entire read-modify-write cycle in _update_manifest_safe - Prevent concurrent writers from racing on manifest updates Other improvements: - Fix pages_v3.py standalone mode to load config.json from disk - Improve error handling with proper logging in cleanup blocks - Add explicit type annotations to Starlark helper functions Co-Authored-By: Claude Sonnet 4.5 --- plugin-repos/starlark-apps/manager.py | 110 +++++++++++++----- .../starlark-apps/tronbyte_repository.py | 21 ++++ web_interface/blueprints/api_v3.py | 18 +-- web_interface/blueprints/pages_v3.py | 12 +- web_interface/static/v3/plugins_manager.js | 25 +++- 5 files changed, 147 insertions(+), 39 deletions(-) diff --git a/plugin-repos/starlark-apps/manager.py b/plugin-repos/starlark-apps/manager.py index a3e71808..b82a256d 100644 --- a/plugin-repos/starlark-apps/manager.py +++ b/plugin-repos/starlark-apps/manager.py @@ -551,39 +551,59 @@ class StarlarkAppsPlugin(BasePlugin): def _save_manifest(self, manifest: Dict[str, Any]) -> bool: """ Save apps manifest to file with file locking to prevent race conditions. - Uses exclusive lock during write to prevent concurrent modifications. + Acquires exclusive lock on manifest file before writing to prevent concurrent modifications. """ temp_file = None + lock_fd = None try: - # Use atomic write pattern: write to temp file, then rename - temp_file = self.manifest_file.with_suffix('.tmp') + # Create parent directory if needed + self.manifest_file.parent.mkdir(parents=True, exist_ok=True) - with open(temp_file, 'w') as f: - # Acquire exclusive lock during write - fcntl.flock(f.fileno(), fcntl.LOCK_EX) - try: + # 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) + + # Acquire exclusive lock on manifest file BEFORE creating temp file + # This serializes all writers and prevents concurrent races + fcntl.flock(lock_fd, fcntl.LOCK_EX) + + try: + # Now that we hold the lock, create and write temp file + temp_file = self.manifest_file.with_suffix('.tmp') + + with open(temp_file, 'w') as f: json.dump(manifest, f, indent=2) f.flush() os.fsync(f.fileno()) # Ensure data is written to disk - finally: - fcntl.flock(f.fileno(), fcntl.LOCK_UN) - # Atomic rename (overwrites destination) - temp_file.replace(self.manifest_file) - return True + # Atomic rename (overwrites destination) while still holding lock + temp_file.replace(self.manifest_file) + return True + finally: + # Release lock + fcntl.flock(lock_fd, fcntl.LOCK_UN) + os.close(lock_fd) + except Exception as e: self.logger.error(f"Error saving manifest: {e}") # Clean up temp file if it exists - if temp_file and temp_file.exists(): + if temp_file is not None and temp_file.exists(): try: temp_file.unlink() - except Exception: - pass + except Exception as cleanup_exc: + self.logger.warning(f"Failed to clean up temp file {temp_file}: {cleanup_exc}") + # Clean up lock fd if still open + if lock_fd is not None: + try: + os.close(lock_fd) + except Exception as cleanup_exc: + self.logger.warning(f"Failed to close lock file descriptor: {cleanup_exc}") return False def _update_manifest_safe(self, updater_fn) -> bool: """ Safely update manifest with file locking to prevent race conditions. + Holds exclusive lock for entire read-modify-write cycle. Args: updater_fn: Function that takes manifest dict and modifies it in-place @@ -591,22 +611,60 @@ class StarlarkAppsPlugin(BasePlugin): Returns: True if successful, False otherwise """ + lock_fd = None + temp_file = None try: - # Read current manifest with shared lock - with open(self.manifest_file, 'r') as f: - fcntl.flock(f.fileno(), fcntl.LOCK_SH) - try: - manifest = json.load(f) - finally: - fcntl.flock(f.fileno(), fcntl.LOCK_UN) + # Create parent directory if needed + self.manifest_file.parent.mkdir(parents=True, exist_ok=True) - # Apply updates - updater_fn(manifest) + # 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) + + # Acquire exclusive lock for entire read-modify-write cycle + fcntl.flock(lock_fd, fcntl.LOCK_EX) + + try: + # Read current manifest while holding exclusive lock + if self.manifest_file.exists() and self.manifest_file.stat().st_size > 0: + with open(self.manifest_file, 'r') as f: + manifest = json.load(f) + else: + # Empty or non-existent file, start with default structure + manifest = {"apps": {}} + + # Apply updates while still holding lock + updater_fn(manifest) + + # Write back to temp file, then atomic replace (still holding lock) + temp_file = self.manifest_file.with_suffix('.tmp') + with open(temp_file, 'w') as f: + json.dump(manifest, f, indent=2) + f.flush() + os.fsync(f.fileno()) + + # Atomic rename while still holding lock + temp_file.replace(self.manifest_file) + return True + + finally: + # Release lock + fcntl.flock(lock_fd, fcntl.LOCK_UN) + os.close(lock_fd) - # Write back with exclusive lock (handled by _save_manifest) - return self._save_manifest(manifest) except Exception as e: self.logger.error(f"Error updating manifest: {e}") + # Clean up temp file if it exists + if temp_file is not None and temp_file.exists(): + try: + temp_file.unlink() + except Exception as cleanup_exc: + self.logger.warning(f"Failed to clean up temp file {temp_file}: {cleanup_exc}") + # Clean up lock fd if still open + if lock_fd is not None: + try: + os.close(lock_fd) + except Exception as cleanup_exc: + self.logger.warning(f"Failed to close lock file descriptor: {cleanup_exc}") return False def update(self) -> None: diff --git a/plugin-repos/starlark-apps/tronbyte_repository.py b/plugin-repos/starlark-apps/tronbyte_repository.py index c00de632..1c1eaca4 100644 --- a/plugin-repos/starlark-apps/tronbyte_repository.py +++ b/plugin-repos/starlark-apps/tronbyte_repository.py @@ -362,6 +362,27 @@ class TronbyteRepository: if '..' in star_filename or '/' in star_filename or '\\' in star_filename: return False, f"Invalid filename: contains path traversal characters" + # Validate output_path to prevent path traversal + import tempfile + try: + resolved_output = output_path.resolve() + temp_dir = Path(tempfile.gettempdir()).resolve() + + # Check if output_path is within the system temp directory + # Use try/except for compatibility with Python < 3.9 (is_relative_to) + try: + is_safe = resolved_output.is_relative_to(temp_dir) + except AttributeError: + # Fallback for Python < 3.9: compare string paths + is_safe = str(resolved_output).startswith(str(temp_dir) + '/') + + if not is_safe: + logger.warning(f"Path traversal attempt in download_star_file: app_id={app_id}, output_path={output_path}") + return False, f"Invalid output_path for {app_id}: must be within temp directory" + except Exception as e: + logger.error(f"Error validating output_path for {app_id}: {e}") + return False, f"Invalid output_path for {app_id}" + # Use provided filename or fall back to app_id.star star_path = f"{self.APPS_PATH}/{app_id}/{star_filename}" diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index eee146b9..76612ca9 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -10,7 +10,7 @@ import uuid import logging from datetime import datetime from pathlib import Path -from typing import Optional +from typing import Optional, Tuple, Dict, Any, Type logger = logging.getLogger(__name__) @@ -6980,7 +6980,7 @@ def clear_old_errors(): # ─── Starlark Apps API ────────────────────────────────────────────────────── -def _get_tronbyte_repository_class(): +def _get_tronbyte_repository_class() -> Type[Any]: """Import TronbyteRepository from plugin-repos directory.""" import importlib.util import importlib @@ -7007,7 +7007,7 @@ def _get_tronbyte_repository_class(): return module.TronbyteRepository -def _get_pixlet_renderer_class(): +def _get_pixlet_renderer_class() -> Type[Any]: """Import PixletRenderer from plugin-repos directory.""" import importlib.util import importlib @@ -7034,7 +7034,7 @@ def _get_pixlet_renderer_class(): return module.PixletRenderer -def _validate_and_sanitize_app_id(app_id, fallback_source=None): +def _validate_and_sanitize_app_id(app_id: Optional[str], fallback_source: Optional[str] = None) -> Tuple[Optional[str], Optional[str]]: """Validate and sanitize app_id to a safe slug.""" if not app_id and fallback_source: app_id = fallback_source @@ -7051,7 +7051,7 @@ def _validate_and_sanitize_app_id(app_id, fallback_source=None): return sanitized, None -def _validate_timing_value(value, field_name, min_val=1, max_val=86400): +def _validate_timing_value(value: Any, field_name: str, min_val: int = 1, max_val: int = 86400) -> Tuple[Optional[int], Optional[str]]: """Validate and coerce timing values.""" if value is None: return None, None @@ -7066,7 +7066,7 @@ def _validate_timing_value(value, field_name, min_val=1, max_val=86400): return int_value, None -def _get_starlark_plugin(): +def _get_starlark_plugin() -> Optional[Any]: """Get the starlark-apps plugin instance, or None.""" if not api_v3.plugin_manager: return None @@ -7078,7 +7078,7 @@ _STARLARK_APPS_DIR = PROJECT_ROOT / 'starlark-apps' _STARLARK_MANIFEST_FILE = _STARLARK_APPS_DIR / 'manifest.json' -def _read_starlark_manifest() -> dict: +def _read_starlark_manifest() -> Dict[str, Any]: """Read the starlark-apps manifest.json directly from disk.""" try: if _STARLARK_MANIFEST_FILE.exists(): @@ -7089,7 +7089,7 @@ def _read_starlark_manifest() -> dict: return {'apps': {}} -def _write_starlark_manifest(manifest: dict) -> bool: +def _write_starlark_manifest(manifest: Dict[str, Any]) -> bool: """Write the starlark-apps manifest.json to disk with atomic write.""" temp_file = None try: @@ -7116,7 +7116,7 @@ def _write_starlark_manifest(manifest: dict) -> bool: return False -def _install_star_file(app_id: str, star_file_path: str, metadata: dict, assets_dir: Optional[str] = None) -> bool: +def _install_star_file(app_id: str, star_file_path: str, metadata: Dict[str, Any], assets_dir: Optional[str] = None) -> bool: """Install a .star file and update the manifest (standalone, no plugin needed).""" import shutil import json diff --git a/web_interface/blueprints/pages_v3.py b/web_interface/blueprints/pages_v3.py index eba6b975..8be1eb24 100644 --- a/web_interface/blueprints/pages_v3.py +++ b/web_interface/blueprints/pages_v3.py @@ -480,6 +480,16 @@ def _load_starlark_config_partial(app_id): except Exception as e: print(f"Warning: Could not load schema for {app_id}: {e}") + # Load config from config.json if it exists + config = {} + config_file = Path(__file__).resolve().parent.parent.parent / 'starlark-apps' / app_id / 'config.json' + if config_file.exists(): + try: + with open(config_file, 'r') as f: + config = json.load(f) + except Exception as e: + print(f"Warning: Could not load config for {app_id}: {e}") + return render_template( 'v3/partials/starlark_config.html', app_id=app_id, @@ -487,7 +497,7 @@ def _load_starlark_config_partial(app_id): app_enabled=app_data.get('enabled', True), render_interval=app_data.get('render_interval', 300), display_duration=app_data.get('display_duration', 15), - config=app_data.get('config', {}), + config=config, schema=schema, has_frames=False, frame_count=0, diff --git a/web_interface/static/v3/plugins_manager.js b/web_interface/static/v3/plugins_manager.js index a032fa6b..b245774e 100644 --- a/web_interface/static/v3/plugins_manager.js +++ b/web_interface/static/v3/plugins_manager.js @@ -7898,7 +7898,7 @@ setTimeout(function() { grid.innerHTML = apps.map(app => { const installed = isStarlarkInstalled(app.id); return ` -
+
@@ -7914,15 +7914,34 @@ setTimeout(function() {
- -
`; }).join(''); + + // Add delegated event listeners for install and view buttons + grid.addEventListener('click', function(e) { + const button = e.target.closest('button[data-action]'); + if (!button) return; + + const card = button.closest('.plugin-card'); + if (!card) return; + + const appId = card.dataset.appId; + if (!appId) return; + + const action = button.dataset.action; + if (action === 'install') { + window.installStarlarkApp(appId); + } else if (action === 'view') { + window.open('https://github.com/tronbyt/apps/tree/main/apps/' + encodeURIComponent(appId), '_blank'); + } + }); } // ── Filter UI Updates ───────────────────────────────────────────────────