diff --git a/plugins/mqtt-notifications b/plugins/mqtt-notifications index 3bd955a32..5c23a842d 160000 --- a/plugins/mqtt-notifications +++ b/plugins/mqtt-notifications @@ -1 +1 @@ -Subproject commit 3bd955a329a9e07933f34098bf26a1f1e0e0c31e +Subproject commit 5c23a842d8efc6e1253bc3dfc67c65ce37a193cc diff --git a/scripts/add_defaults_to_schemas.py b/scripts/add_defaults_to_schemas.py new file mode 100755 index 000000000..9460c9cad --- /dev/null +++ b/scripts/add_defaults_to_schemas.py @@ -0,0 +1,231 @@ +#!/usr/bin/env python3 +""" +Script to add default values to plugin config schemas where missing. + +This ensures that configs never start with None values, improving user experience +and preventing validation errors. +""" + +import json +import sys +from pathlib import Path +from typing import Any, Dict, List, Optional + + +def get_default_for_field(prop: Dict[str, Any]) -> Any: + """ + Determine a sensible default value for a field based on its type and constraints. + + Args: + prop: Field property schema + + Returns: + Default value or None if no default should be added + """ + prop_type = prop.get('type') + + # Handle union types (array with multiple types) + if isinstance(prop_type, list): + # Use the first non-null type + prop_type = next((t for t in prop_type if t != 'null'), prop_type[0] if prop_type else 'string') + + if prop_type == 'boolean': + return False + + elif prop_type == 'number': + # For numbers, use minimum if available, or a sensible default + minimum = prop.get('minimum') + maximum = prop.get('maximum') + + if minimum is not None: + return minimum + elif maximum is not None: + # Use a reasonable fraction of max (like 30% or minimum 1) + return max(1, int(maximum * 0.3)) + else: + # No constraints, use 0 + return 0 + + elif prop_type == 'integer': + # Similar to number + minimum = prop.get('minimum') + maximum = prop.get('maximum') + + if minimum is not None: + return minimum + elif maximum is not None: + return max(1, int(maximum * 0.3)) + else: + return 0 + + elif prop_type == 'string': + # Only add default for strings if it makes sense + # Check if there's an enum - use first value + enum_values = prop.get('enum') + if enum_values: + return enum_values[0] + + # For optional string fields, empty string might be okay, but be cautious + # We'll skip adding defaults for strings unless explicitly needed + return None + + elif prop_type == 'array': + # Empty array as default + return [] + + elif prop_type == 'object': + # Empty object - but we'll handle nested objects separately + return {} + + return None + + +def should_add_default(prop: Dict[str, Any], field_path: str) -> bool: + """ + Determine if we should add a default value to this field. + + Args: + prop: Field property schema + field_path: Dot-separated path to the field + + Returns: + True if default should be added + """ + # Skip if already has a default + if 'default' in prop: + return False + + # Skip secret fields (they should be user-provided) + if prop.get('x-secret', False): + return False + + # Skip API keys and similar sensitive fields + field_name = field_path.split('.')[-1].lower() + sensitive_keywords = ['key', 'password', 'secret', 'token', 'auth', 'credential'] + if any(keyword in field_name for keyword in sensitive_keywords): + return False + + prop_type = prop.get('type') + if isinstance(prop_type, list): + prop_type = next((t for t in prop_type if t != 'null'), prop_type[0] if prop_type else None) + + # Only add defaults for certain types + if prop_type in ('boolean', 'number', 'integer', 'array'): + return True + + # For strings, only if there's an enum + if prop_type == 'string' and 'enum' in prop: + return True + + return False + + +def add_defaults_recursive(schema: Dict[str, Any], path: str = "", modified: List[str] = None) -> bool: + """ + Recursively add default values to schema fields. + + Args: + schema: Schema dictionary to modify + path: Current path in the schema (for logging) + modified: List to track which fields were modified + + Returns: + True if any modifications were made + """ + if modified is None: + modified = [] + + if not isinstance(schema, dict) or 'properties' not in schema: + return False + + changes_made = False + + for key, prop in schema['properties'].items(): + if not isinstance(prop, dict): + continue + + current_path = f"{path}.{key}" if path else key + + # Check nested objects + if prop.get('type') == 'object' and 'properties' in prop: + if add_defaults_recursive(prop, current_path, modified): + changes_made = True + + # Add default if appropriate + if should_add_default(prop, current_path): + default_value = get_default_for_field(prop) + if default_value is not None: + prop['default'] = default_value + modified.append(current_path) + changes_made = True + print(f" Added default to {current_path}: {default_value} (type: {prop.get('type')})") + + return changes_made + + +def process_schema_file(schema_path: Path) -> bool: + """ + Process a single schema file to add defaults. + + Args: + schema_path: Path to the schema file + + Returns: + True if file was modified + """ + print(f"\nProcessing: {schema_path}") + + try: + with open(schema_path, 'r', encoding='utf-8') as f: + schema = json.load(f) + except Exception as e: + print(f" Error reading schema: {e}") + return False + + modified_fields = [] + changes_made = add_defaults_recursive(schema, modified=modified_fields) + + if changes_made: + # Write back with pretty formatting + with open(schema_path, 'w', encoding='utf-8') as f: + json.dump(schema, f, indent=2, ensure_ascii=False) + f.write('\n') # Add trailing newline + + print(f" ✓ Modified {len(modified_fields)} fields") + return True + else: + print(f" ✓ No changes needed") + return False + + +def main(): + """Main entry point.""" + project_root = Path(__file__).parent.parent + plugins_dir = project_root / 'plugins' + + if not plugins_dir.exists(): + print(f"Error: Plugins directory not found: {plugins_dir}") + sys.exit(1) + + # Find all config_schema.json files + schema_files = list(plugins_dir.rglob('config_schema.json')) + + if not schema_files: + print("No config_schema.json files found") + sys.exit(0) + + print(f"Found {len(schema_files)} schema files") + + modified_count = 0 + for schema_file in sorted(schema_files): + if process_schema_file(schema_file): + modified_count += 1 + + print(f"\n{'='*60}") + print(f"Summary: Modified {modified_count} out of {len(schema_files)} schema files") + print(f"{'='*60}") + + +if __name__ == '__main__': + main() + diff --git a/src/plugin_system/schema_manager.py b/src/plugin_system/schema_manager.py index 72d701d81..9cd9fd2d3 100644 --- a/src/plugin_system/schema_manager.py +++ b/src/plugin_system/schema_manager.py @@ -394,24 +394,54 @@ def _format_validation_error(self, error: ValidationError, plugin_id: Optional[s def merge_with_defaults(self, config: Dict[str, Any], defaults: Dict[str, Any]) -> Dict[str, Any]: """ Merge configuration with defaults, preserving user values. + Also replaces None values with defaults to ensure config never has None from the start. Args: config: User configuration defaults: Default values from schema Returns: - Merged configuration with defaults applied where missing + Merged configuration with defaults applied where missing or None """ - merged = defaults.copy() + merged = copy.deepcopy(defaults) - def deep_merge(target: Dict[str, Any], source: Dict[str, Any]) -> None: - """Recursively merge source into target.""" + def deep_merge(target: Dict[str, Any], source: Dict[str, Any], default_dict: Dict[str, Any]) -> None: + """Recursively merge source into target, replacing None with defaults.""" for key, value in source.items(): + default_value = default_dict.get(key) + if key in target and isinstance(target[key], dict) and isinstance(value, dict): - deep_merge(target[key], value) + # Both are dicts, recursively merge + if isinstance(default_value, dict): + deep_merge(target[key], value, default_value) + else: + deep_merge(target[key], value, {}) + elif value is None and default_value is not None: + # Value is None and we have a default, use the default + target[key] = copy.deepcopy(default_value) if isinstance(default_value, (dict, list)) else default_value else: - target[key] = value - - deep_merge(merged, config) + # Normal merge: user value takes precedence (copy if dict/list) + if isinstance(value, (dict, list)): + target[key] = copy.deepcopy(value) + else: + target[key] = value + + deep_merge(merged, config, defaults) + + # Final pass: replace any remaining None values at any level with defaults + def replace_none_with_defaults(target: Dict[str, Any], default_dict: Dict[str, Any]) -> None: + """Recursively replace None values with defaults.""" + for key in list(target.keys()): + value = target[key] + default_value = default_dict.get(key) + + if value is None and default_value is not None: + # Replace None with default + target[key] = copy.deepcopy(default_value) if isinstance(default_value, (dict, list)) else default_value + elif isinstance(value, dict) and isinstance(default_value, dict): + # Recursively process nested dicts + replace_none_with_defaults(value, default_value) + + replace_none_with_defaults(merged, defaults) return merged diff --git a/web_interface/app.py b/web_interface/app.py index e0652ae01..6fdf7067f 100644 --- a/web_interface/app.py +++ b/web_interface/app.py @@ -26,30 +26,14 @@ app.secret_key = os.urandom(24) config_manager = ConfigManager() -# Initialize CSRF protection (optional for local-only, but recommended for defense-in-depth) -try: - from flask_wtf.csrf import CSRFProtect - csrf = CSRFProtect(app) - # Exempt SSE streams from CSRF (read-only) - from functools import wraps - from flask import request - - def csrf_exempt(f): - """Decorator to exempt a route from CSRF protection.""" - f.csrf_exempt = True - return f - - # Mark SSE streams as exempt - @app.before_request - def check_csrf_exempt(): - """Check if route should be exempt from CSRF.""" - if request.endpoint and 'stream' in request.endpoint: - # SSE streams are read-only, exempt from CSRF - pass -except ImportError: - # flask-wtf not installed, CSRF protection disabled - csrf = None - pass +# CSRF protection disabled for local-only application +# CSRF is designed for internet-facing web apps to prevent cross-site request forgery. +# For a local-only Raspberry Pi application, the threat model is different: +# - If an attacker has network access to perform CSRF, they have other attack vectors +# - All API endpoints are programmatic (HTMX/fetch) and don't include CSRF tokens +# - Forms use HTMX which doesn't automatically include CSRF tokens +# If you need CSRF protection (e.g., exposing to internet), properly implement CSRF tokens in HTMX forms +csrf = None # Initialize rate limiting (prevent accidental abuse, not security) try: @@ -543,6 +527,7 @@ def stream_logs(): csrf.exempt(stream_stats) csrf.exempt(stream_display) csrf.exempt(stream_logs) + # Note: api_v3 blueprint is exempted above after registration if limiter: limiter.limit("20 per minute")(stream_stats) diff --git a/web_interface/blueprints/api_v3.py b/web_interface/blueprints/api_v3.py index 473689dad..fdf180d94 100644 --- a/web_interface/blueprints/api_v3.py +++ b/web_interface/blueprints/api_v3.py @@ -632,7 +632,12 @@ def separate_secrets(config, secrets_set, prefix=''): import traceback error_msg = f"Error saving config: {str(e)}\n{traceback.format_exc()}" logging.error(error_msg) - return jsonify({'status': 'error', 'message': str(e)}), 500 + return error_response( + ErrorCode.CONFIG_SAVE_FAILED, + f"Error saving configuration: {str(e)}", + details=traceback.format_exc(), + status_code=500 + ) @api_v3.route('/config/secrets', methods=['GET']) def get_secrets_config(): @@ -675,14 +680,24 @@ def save_raw_main_config(): # Extract more specific error message if it's a ConfigError if isinstance(e, ConfigError): - # ConfigError has a message attribute and may have context error_message = str(e) if hasattr(e, 'config_path') and e.config_path: error_message = f"{error_message} (config_path: {e.config_path})" + return error_response( + ErrorCode.CONFIG_SAVE_FAILED, + error_message, + details=traceback.format_exc(), + context={'config_path': e.config_path} if hasattr(e, 'config_path') and e.config_path else None, + status_code=500 + ) else: error_message = str(e) if str(e) else "An unexpected error occurred while saving the configuration" - - return jsonify({'status': 'error', 'message': error_message}), 500 + return error_response( + ErrorCode.UNKNOWN_ERROR, + error_message, + details=traceback.format_exc(), + status_code=500 + ) @api_v3.route('/config/raw/secrets', methods=['POST']) def save_raw_secrets_config(): @@ -2899,6 +2914,43 @@ def _get_schema_property(schema, key_path): return None +def _is_field_required(key_path, schema): + """ + Check if a field is required according to the schema. + + Args: + key_path: Dot-separated path like "mqtt.username" + schema: The JSON schema dict + + Returns: + True if field is required, False otherwise + """ + if not schema or 'properties' not in schema: + return False + + parts = key_path.split('.') + if len(parts) == 1: + # Top-level field + required = schema.get('required', []) + return parts[0] in required + else: + # Nested field - navigate to parent object + parent_path = '.'.join(parts[:-1]) + field_name = parts[-1] + + # Get parent property + parent_prop = _get_schema_property(schema, parent_path) + if not parent_prop or 'properties' not in parent_prop: + return False + + # Check if field is required in parent + required = parent_prop.get('required', []) + return field_name in required + + +# Sentinel object to indicate a field should be skipped (not set in config) +_SKIP_FIELD = object() + def _parse_form_value_with_schema(value, key_path, schema): """ Parse a form value using schema information to determine correct type. @@ -2910,7 +2962,7 @@ def _parse_form_value_with_schema(value, key_path, schema): schema: The plugin's JSON schema Returns: - Parsed value with correct type + Parsed value with correct type, or _SKIP_FIELD to indicate the field should not be set """ import json @@ -2925,6 +2977,22 @@ def _parse_form_value_with_schema(value, key_path, schema): # If schema says it's an object, return empty dict instead of None if prop and prop.get('type') == 'object': return {} + # If it's an optional string field, preserve empty string instead of None + if prop and prop.get('type') == 'string': + if not _is_field_required(key_path, schema): + return "" # Return empty string for optional string fields + # For number/integer fields, check if they have defaults or are required + if prop: + prop_type = prop.get('type') + if prop_type in ('number', 'integer'): + # If field has a default, use it + if 'default' in prop: + return prop['default'] + # If field is not required and has no default, skip setting it + if not _is_field_required(key_path, schema): + return _SKIP_FIELD + # If field is required but empty, return None (validation will fail, which is correct) + return None return None # Handle string values @@ -3014,8 +3082,12 @@ def _set_nested_value(config, key_path, value): Args: config: The config dict to modify key_path: Dot-separated path (e.g., "customization.period_text.font") - value: The value to set + value: The value to set (or _SKIP_FIELD to skip setting) """ + # Skip setting if value is the sentinel + if value is _SKIP_FIELD: + return + parts = key_path.split('.') current = config @@ -3216,7 +3288,9 @@ def save_plugin_config(): import logging logger = logging.getLogger(__name__) logger.debug(f"Combined indexed array field {base_path}: {values} -> {combined_value} -> {parsed_value}") - _set_nested_value(plugin_config, base_path, parsed_value) + # Only set if not skipped + if parsed_value is not _SKIP_FIELD: + _set_nested_value(plugin_config, base_path, parsed_value) # Process remaining (non-indexed) fields # Skip any base paths that were processed as indexed arrays @@ -3234,8 +3308,9 @@ def save_plugin_config(): import logging logger = logging.getLogger(__name__) logger.debug(f"Array field {key}: form value='{value}' -> parsed={parsed_value}") - # Use helper to set nested values correctly - _set_nested_value(plugin_config, key, parsed_value) + # Use helper to set nested values correctly (skips if _SKIP_FIELD) + if parsed_value is not _SKIP_FIELD: + _set_nested_value(plugin_config, key, parsed_value) # Post-process: Fix array fields that might have been incorrectly structured # This handles cases where array fields are stored as dicts (e.g., from indexed form fields) diff --git a/web_interface/templates/v3/base.html b/web_interface/templates/v3/base.html index 8cc9c7c64..03a3fa783 100644 --- a/web_interface/templates/v3/base.html +++ b/web_interface/templates/v3/base.html @@ -4209,13 +4209,28 @@

const btn = event.target.querySelector('[type=submit]'); if (btn) btn.disabled = false; - if (event.detail.successful) { - showNotification('Configuration saved successfully!', 'success'); + const xhr = event.detail.xhr; + const status = xhr?.status || 0; + + // Check if request was successful (2xx status codes) + if (status >= 200 && status < 300) { + // Try to get message from response JSON + let message = 'Configuration saved successfully!'; + try { + if (xhr?.responseJSON?.message) { + message = xhr.responseJSON.message; + } else if (xhr?.responseText) { + const responseData = JSON.parse(xhr.responseText); + message = responseData.message || message; + } + } catch (e) { + // Use default message if parsing fails + } + showNotification(message, 'success'); } else { - // Log detailed error information - const xhr = event.detail.xhr; + // Request failed - log detailed error information console.error('Config save failed:', { - status: xhr?.status, + status: status, statusText: xhr?.statusText, responseText: xhr?.responseText }); @@ -4223,7 +4238,13 @@

// Try to parse error response let errorMessage = 'Failed to save configuration'; try { - if (xhr?.responseText) { + if (xhr?.responseJSON) { + const errorData = xhr.responseJSON; + errorMessage = errorData.message || errorData.details || errorMessage; + if (errorData.validation_errors) { + errorMessage += ': ' + errorData.validation_errors.join(', '); + } + } else if (xhr?.responseText) { const errorData = JSON.parse(xhr.responseText); errorMessage = errorData.message || errorData.details || errorMessage; if (errorData.validation_errors) { @@ -4241,7 +4262,10 @@

// Handle toggle response window.handleToggleResponse = function(event, pluginId) { - if (event.detail.successful) { + const xhr = event.detail.xhr; + const status = xhr?.status || 0; + + if (status >= 200 && status < 300) { // Update UI in place instead of refreshing to avoid duplication const checkbox = document.getElementById(`plugin-enabled-${pluginId}`); const label = checkbox?.nextElementSibling; @@ -4252,14 +4276,39 @@

label.className = `ml-2 text-sm ${isEnabled ? 'text-green-600' : 'text-gray-500'}`; } - showNotification('Plugin status updated', 'success'); + // Try to get message from response + let message = 'Plugin status updated'; + try { + if (xhr?.responseJSON?.message) { + message = xhr.responseJSON.message; + } else if (xhr?.responseText) { + const responseData = JSON.parse(xhr.responseText); + message = responseData.message || message; + } + } catch (e) { + // Use default message + } + showNotification(message, 'success'); } else { // Revert checkbox state on error const checkbox = document.getElementById(`plugin-enabled-${pluginId}`); if (checkbox) { checkbox.checked = !checkbox.checked; } - showNotification('Failed to update plugin status', 'error'); + + // Try to get error message from response + let errorMessage = 'Failed to update plugin status'; + try { + if (xhr?.responseJSON?.message) { + errorMessage = xhr.responseJSON.message; + } else if (xhr?.responseText) { + const errorData = JSON.parse(xhr.responseText); + errorMessage = errorData.message || errorData.details || errorMessage; + } + } catch (e) { + // Use default message + } + showNotification(errorMessage, 'error'); } }; diff --git a/web_interface/templates/v3/partials/fonts.html b/web_interface/templates/v3/partials/fonts.html index f3035a82a..bae50438c 100644 --- a/web_interface/templates/v3/partials/fonts.html +++ b/web_interface/templates/v3/partials/fonts.html @@ -190,23 +190,37 @@

Font Preview