From 885fdeed62028d41d53edae5d4ced967f23bc1ad Mon Sep 17 00:00:00 2001 From: Chuck Date: Wed, 18 Feb 2026 21:38:57 -0500 Subject: [PATCH] feat(starlark): schema-driven config forms + critical security fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Schema-Driven Config UI - Render type-appropriate form inputs from schema.json (text, dropdown, toggle, color, datetime, location) - Pre-populate config.json with schema defaults on install - Auto-merge schema defaults when loading existing apps (handles schema updates) - Location fields: 3-part mini-form (lat/lng/timezone) assembles into JSON - Toggle fields: support both boolean and string "true"/"false" values - Unsupported field types (oauth2, photo_select) show warning banners - Fallback to raw key/value inputs for apps without schema ## Critical Security Fixes (P0) - **Path Traversal**: Verify path safety BEFORE mkdir to prevent TOCTOU - **Race Conditions**: Add file locking (fcntl) + atomic writes to manifest operations - **Command Injection**: Validate config keys/values with regex before passing to Pixlet subprocess ## Major Logic Fixes (P1) - **Config/Manifest Separation**: Store timing keys (render_interval, display_duration) ONLY in manifest - **Location Validation**: Validate lat [-90,90] and lng [-180,180] ranges, reject malformed JSON - **Schema Defaults Merge**: Auto-apply new schema defaults to existing app configs on load - **Config Key Validation**: Enforce alphanumeric+underscore format, prevent prototype pollution ## Files Changed - web_interface/templates/v3/partials/starlark_config.html — schema-driven form rendering - plugin-repos/starlark-apps/manager.py — file locking, path safety, config validation, schema merge - plugin-repos/starlark-apps/pixlet_renderer.py — config value sanitization - web_interface/blueprints/api_v3.py — timing key separation, safe manifest updates Co-Authored-By: Claude Sonnet 4.5 --- plugin-repos/starlark-apps/manager.py | 158 +++++++- plugin-repos/starlark-apps/pixlet_renderer.py | 13 + web_interface/blueprints/api_v3.py | 38 +- .../v3/partials/starlark_config.html | 350 ++++++++++++++++-- 4 files changed, 502 insertions(+), 57 deletions(-) diff --git a/plugin-repos/starlark-apps/manager.py b/plugin-repos/starlark-apps/manager.py index 04a915ee..825dcc12 100644 --- a/plugin-repos/starlark-apps/manager.py +++ b/plugin-repos/starlark-apps/manager.py @@ -11,6 +11,7 @@ import json import os import re import time +import fcntl from pathlib import Path from typing import Dict, Any, Optional, List, Tuple from PIL import Image @@ -43,10 +44,13 @@ class StarlarkApp: self.schema_file = app_dir / "schema.json" self.cache_file = app_dir / "cached_render.webp" - # Load app configuration + # Load app configuration and schema self.config = self._load_config() self.schema = self._load_schema() + # Merge schema defaults into config for any missing fields + self._merge_schema_defaults() + # Runtime state self.frames: Optional[List[Tuple[Image.Image, int]]] = None self.current_frame_index = 0 @@ -73,9 +77,70 @@ class StarlarkApp: logger.warning(f"Could not load schema for {self.app_id}: {e}") return None + def _merge_schema_defaults(self) -> None: + """ + Merge schema default values into config for any missing fields. + This ensures existing apps get defaults when schemas are updated with new fields. + """ + if not self.schema: + return + + # Get fields from schema (handles both 'fields' and 'schema' keys) + fields = self.schema.get('fields') or self.schema.get('schema') or [] + defaults_added = False + + for field in fields: + if isinstance(field, dict) and 'id' in field and 'default' in field: + field_id = field['id'] + # Only add if not already present in config + if field_id not in self.config: + self.config[field_id] = field['default'] + defaults_added = True + logger.debug(f"Added default value for {self.app_id}.{field_id}: {field['default']}") + + # Save config if we added any defaults + if defaults_added: + self.save_config() + + def _validate_config(self) -> Optional[str]: + """ + Validate config values to prevent injection and ensure data integrity. + + Returns: + Error message if validation fails, None if valid + """ + for key, value in self.config.items(): + # Validate key format + if not re.match(r'^[a-zA-Z_][a-zA-Z0-9_]{0,63}$', key): + return f"Invalid config key format: {key}" + + # Validate location fields (JSON format) + if isinstance(value, str) and value.strip().startswith('{'): + try: + loc = json.loads(value) + if 'lat' in loc: + lat = float(loc['lat']) + if not -90 <= lat <= 90: + return f"Latitude {lat} out of range [-90, 90] for key {key}" + if 'lng' in loc: + lng = float(loc['lng']) + if not -180 <= lng <= 180: + return f"Longitude {lng} out of range [-180, 180] for key {key}" + except (json.JSONDecodeError, ValueError, KeyError) as e: + # Not a location field, that's fine + pass + + return None + def save_config(self) -> bool: - """Save current configuration to file.""" + """Save current configuration to file with validation.""" try: + # Validate config before saving + error = self._validate_config() + if error: + logger.error(f"Config validation failed for {self.app_id}: {error}") + return False + with open(self.config_file, 'w') as f: json.dump(self.config, f, indent=2) return True @@ -484,13 +549,63 @@ class StarlarkAppsPlugin(BasePlugin): self.logger.exception(f"Error loading apps manifest: {e}") def _save_manifest(self, manifest: Dict[str, Any]) -> bool: - """Save apps manifest to file.""" + """ + Save apps manifest to file with file locking to prevent race conditions. + Uses exclusive lock during write to prevent concurrent modifications. + """ try: - with open(self.manifest_file, 'w') as f: - json.dump(manifest, f, indent=2) + # Use atomic write pattern: write to temp file, then rename + temp_file = self.manifest_file.with_suffix('.tmp') + + with open(temp_file, 'w') as f: + # Acquire exclusive lock during write + fcntl.flock(f.fileno(), fcntl.LOCK_EX) + try: + 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 except Exception as e: self.logger.error(f"Error saving manifest: {e}") + # Clean up temp file if it exists + if temp_file.exists(): + try: + temp_file.unlink() + except: + pass + return False + + def _update_manifest_safe(self, updater_fn) -> bool: + """ + Safely update manifest with file locking to prevent race conditions. + + Args: + updater_fn: Function that takes manifest dict and modifies it in-place + + Returns: + True if successful, False otherwise + """ + 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) + + # Apply updates + updater_fn(manifest) + + # 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}") return False def update(self) -> None: @@ -585,10 +700,14 @@ class StarlarkAppsPlugin(BasePlugin): magnify = self._get_effective_magnify() self.logger.debug(f"Using magnify={magnify} for {app.app_id}") + # Filter out LEDMatrix-internal timing keys before passing to pixlet + INTERNAL_KEYS = {'render_interval', 'display_duration'} + pixlet_config = {k: v for k, v in app.config.items() if k not in INTERNAL_KEYS} + success, error = self.pixlet.render( star_file=str(app.star_file), output_path=str(app.cache_file), - config=app.config, + config=pixlet_config, magnify=magnify ) @@ -697,10 +816,10 @@ class StarlarkAppsPlugin(BasePlugin): # Create app directory with resolved path app_dir = (self.apps_dir / safe_app_id).resolve() - app_dir.mkdir(parents=True, exist_ok=True) - # Verify path safety after mkdir + # Verify path safety BEFORE creating directories self._verify_path_safety(app_dir, self.apps_dir) + app_dir.mkdir(parents=True, exist_ok=True) # Copy .star file with sanitized app_id star_dest = app_dir / f"{safe_app_id}.star" @@ -727,8 +846,13 @@ class StarlarkAppsPlugin(BasePlugin): with open(schema_path, 'w') as f: json.dump(schema, f, indent=2) - # Create default config + # Create default config — pre-populate with schema defaults default_config = {} + if schema: + fields = schema.get('fields') or schema.get('schema') or [] + for field in fields: + if isinstance(field, dict) and 'id' in field and 'default' in field: + default_config[field['id']] = field['default'] config_path = app_dir / "config.json" # Verify config path safety self._verify_path_safety(config_path, self.apps_dir) @@ -736,11 +860,10 @@ class StarlarkAppsPlugin(BasePlugin): json.dump(default_config, f, indent=2) # Update manifest (use safe_app_id as key to match directory) - with open(self.manifest_file, 'r') as f: - manifest = json.load(f) + def update_fn(manifest): + manifest["apps"][safe_app_id] = app_manifest - manifest["apps"][safe_app_id] = app_manifest - self._save_manifest(manifest) + self._update_manifest_safe(update_fn) # Create app instance (use safe_app_id for internal key, original for display) app = StarlarkApp(safe_app_id, app_dir, app_manifest) @@ -782,12 +905,11 @@ class StarlarkAppsPlugin(BasePlugin): shutil.rmtree(app.app_dir) # Update manifest - with open(self.manifest_file, 'r') as f: - manifest = json.load(f) + def update_fn(manifest): + if app_id in manifest["apps"]: + del manifest["apps"][app_id] - if app_id in manifest["apps"]: - del manifest["apps"][app_id] - self._save_manifest(manifest) + self._update_manifest_safe(update_fn) self.logger.info(f"Uninstalled Starlark app: {app_id}") return True diff --git a/plugin-repos/starlark-apps/pixlet_renderer.py b/plugin-repos/starlark-apps/pixlet_renderer.py index 29805331..808b66c0 100644 --- a/plugin-repos/starlark-apps/pixlet_renderer.py +++ b/plugin-repos/starlark-apps/pixlet_renderer.py @@ -9,6 +9,7 @@ import json import logging import os import platform +import re import shutil import subprocess from pathlib import Path @@ -250,11 +251,23 @@ class PixletRenderer: # Add configuration parameters if config: for key, value in config.items(): + # Validate key format (alphanumeric + underscore only) + if not re.match(r'^[a-zA-Z_][a-zA-Z0-9_]*$', key): + logger.warning(f"Skipping invalid config key: {key}") + continue + # Convert value to string for CLI if isinstance(value, bool): value_str = "true" if value else "false" else: value_str = str(value) + + # Validate value doesn't contain shell metacharacters + # Allow alphanumeric, spaces, and common safe chars: .-_:/@#, + if not re.match(r'^[a-zA-Z0-9 .\-_:/@#,{}"\[\]]*$', value_str): + logger.warning(f"Skipping config value with unsafe characters for key {key}: {value_str}") + continue + cmd.extend(["-c", f"{key}={value_str}"]) logger.debug(f"Executing Pixlet: {' '.join(cmd)}") diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 52f16a83..1fc251e8 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -7374,8 +7374,38 @@ def update_starlark_app_config(app_id): app = starlark_plugin.apps.get(app_id) if not app: return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404 + + # Extract timing keys from data before updating config (they belong in manifest, not config) + render_interval = data.pop('render_interval', None) + display_duration = data.pop('display_duration', None) + + # Update config with non-timing fields only app.config.update(data) + + # Update manifest with timing fields + timing_changed = False + if render_interval is not None: + app.manifest['render_interval'] = render_interval + timing_changed = True + 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: @@ -7418,10 +7448,10 @@ def toggle_starlark_app(app_id): if enabled is None: enabled = not app.is_enabled() app.manifest['enabled'] = enabled - with open(starlark_plugin.manifest_file, 'r') as f: - manifest = json.load(f) - manifest['apps'][app_id]['enabled'] = enabled - starlark_plugin._save_manifest(manifest) + # Use safe manifest update to prevent race conditions + def update_fn(manifest): + manifest['apps'][app_id]['enabled'] = enabled + starlark_plugin._update_manifest_safe(update_fn) return jsonify({'status': 'success', 'message': f"App {'enabled' if enabled else 'disabled'}", 'enabled': enabled}) # Standalone: update manifest directly diff --git a/web_interface/templates/v3/partials/starlark_config.html b/web_interface/templates/v3/partials/starlark_config.html index 8be5d1fb..f743f3b3 100644 --- a/web_interface/templates/v3/partials/starlark_config.html +++ b/web_interface/templates/v3/partials/starlark_config.html @@ -53,7 +53,7 @@ - +

Timing Settings

@@ -76,18 +76,239 @@
- {% if schema or config %} + {# ── Schema-driven App Settings ── #} + {% set fields = [] %} + {% if schema %} + {% if schema.fields is defined %} + {% set fields = schema.fields %} + {% elif schema.schema is defined and schema.schema is iterable and schema.schema is not string %} + {% set fields = schema.schema %} + {% endif %} + {% endif %} + + {% if fields %} +
+

App Settings

+ + {% for field in fields %} + {% if field.typeOf is defined and field.id is defined %} + {% set field_id = field.id %} + {% set field_type = field.typeOf %} + {% set field_name = field.name or field_id %} + {% set field_desc = field.desc or '' %} + {% set field_default = field.default if field.default is defined else '' %} + {% set current_val = config.get(field_id, field_default) if config else field_default %} + + {# ── text ── #} + {% if field_type == 'text' %} +
+ + + {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ + {# ── dropdown ── #} + {% elif field_type == 'dropdown' %} +
+ + + {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ + {# ── toggle ── #} + {% elif field_type == 'toggle' %} +
+ + {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ + {# ── color ── #} + {% elif field_type == 'color' %} +
+ +
+ + +
+ {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ + {# ── datetime ── #} + {% elif field_type == 'datetime' %} +
+ + + {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ + {# ── location (mini-form) ── #} + {% elif field_type == 'location' %} +
+ +
+
+ +
+
+ +
+
+ +
+
+ {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ + {# ── oauth2 (unsupported) ── #} + {% elif field_type == 'oauth2' %} +
+ +
+ + This app requires OAuth2 authentication, which is not supported in standalone mode. +
+ {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ + {# ── photo_select (unsupported) ── #} + {% elif field_type == 'photo_select' %} +
+ +
+ + Photo upload is not supported in this interface. +
+
+ + {# ── generated (hidden meta-field, skip) ── #} + {% elif field_type == 'generated' %} + {# Invisible — generated fields are handled server-side by Pixlet #} + + {# ── typeahead / location_based (text fallback with note) ── #} + {% elif field_type in ('typeahead', 'location_based') %} +
+ + +

+ + This field normally uses autocomplete which requires a Pixlet server. Enter the value manually. +

+ {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ + {# ── unknown type (text fallback) ── #} + {% else %} +
+ + + {% if field_desc %} +

{{ field_desc }}

+ {% endif %} +
+ {% endif %} + + {% endif %}{# end field.typeOf and field.id check #} + {% endfor %} + + {# Also show any config keys NOT in the schema (user-added or legacy) #} + {% if config %} + {% set schema_ids = [] %} + {% for f in fields %} + {% if f.id is defined %} + {% if schema_ids.append(f.id) %}{% endif %} + {% endif %} + {% endfor %} + {% for key, value in config.items() %} + {% if key not in ('render_interval', 'display_duration') and key not in schema_ids %} +
+ + +
+ {% endif %} + {% endfor %} + {% endif %} + + {# ── No schema: fall back to raw config key/value pairs ── #} + {% elif config %}

App Settings

{% for key, value in config.items() %} - {% if key not in ('render_interval', 'display_duration') %} -
- - -
- {% endif %} + {% if key not in ('render_interval', 'display_duration') %} +
+ + +
+ {% endif %} {% endfor %} {% endif %} @@ -102,15 +323,25 @@