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 <noreply@anthropic.com>

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
+            # Create parent directory if needed
-            temp_file = self.manifest_file.with_suffix('.tmp')
+            self.manifest_file.parent.mkdir(parents=True, exist_ok=True)
 
-            with open(temp_file, 'w') as f:
+            # Open manifest file for locking (create if doesn't exist, don't truncate)
-                # Acquire exclusive lock during write
+            # Use os.open with O_CREAT | O_RDWR to create if missing, but don't truncate
-                fcntl.flock(f.fileno(), fcntl.LOCK_EX)
+            lock_fd = os.open(str(self.manifest_file), os.O_CREAT | os.O_RDWR, 0o644)
-                try:
+
+            # 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)
+                # Atomic rename (overwrites destination) while still holding lock
-            temp_file.replace(self.manifest_file)
+                temp_file.replace(self.manifest_file)
-            return True
+                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:
+                except Exception as cleanup_exc:
-                    pass
+                    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
+            # Create parent directory if needed
-            with open(self.manifest_file, 'r') as f:
+            self.manifest_file.parent.mkdir(parents=True, exist_ok=True)
-                fcntl.flock(f.fileno(), fcntl.LOCK_SH)
-                try:
-                    manifest = json.load(f)
-                finally:
-                    fcntl.flock(f.fileno(), fcntl.LOCK_UN)
 
-            # Apply updates
+            # Open manifest file for locking (create if doesn't exist, don't truncate)
-            updater_fn(manifest)
+            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:

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}"
 

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

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,

web_interface/static/v3/plugins_manager.js

@@ -7898,7 +7898,7 @@ setTimeout(function() {
         grid.innerHTML = apps.map(app => {
             const installed = isStarlarkInstalled(app.id);
             return `
-            <div class="plugin-card">
+            <div class="plugin-card" data-app-id="${escapeHtml(app.id)}">
                 <div class="flex items-start justify-between mb-4">
                     <div class="flex-1 min-w-0">
                         <div class="flex items-center flex-wrap gap-1.5 mb-2">
@@ -7914,15 +7914,34 @@ setTimeout(function() {
                     </div>
                 </div>
                 <div class="flex gap-2 mt-auto pt-3 border-t border-gray-200">
-                    <button onclick="window.installStarlarkApp('${escapeHtml(app.id)}')" class="btn ${installed ? 'bg-gray-500 hover:bg-gray-600' : 'bg-green-600 hover:bg-green-700'} text-white px-4 py-2 rounded-md text-sm font-semibold flex-1 flex justify-center items-center">
+                    <button data-action="install" class="btn ${installed ? 'bg-gray-500 hover:bg-gray-600' : 'bg-green-600 hover:bg-green-700'} text-white px-4 py-2 rounded-md text-sm font-semibold flex-1 flex justify-center items-center">
                         <i class="fas ${installed ? 'fa-redo' : 'fa-download'} mr-2"></i>${installed ? 'Reinstall' : 'Install'}
                     </button>
-                    <button onclick="window.open('https://github.com/tronbyt/apps/tree/main/apps/${encodeURIComponent(app.id)}', '_blank')" class="btn bg-gray-600 hover:bg-gray-700 text-white px-4 py-2 rounded-md text-sm font-semibold flex justify-center items-center">
+                    <button data-action="view" class="btn bg-gray-600 hover:bg-gray-700 text-white px-4 py-2 rounded-md text-sm font-semibold flex justify-center items-center">
                         <i class="fas fa-external-link-alt mr-1"></i>View
                     </button>
                 </div>
             </div>`;
         }).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 ───────────────────────────────────────────────────