From cd7e16e58ec6ebbe2ad30f843a21b63ef10f6db4 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Tue, 14 Jul 2026 17:23:25 -0400 Subject: [PATCH] fix(security): validate plugin_id before path construction in /api/install CodeQL flagged 16 high-severity "path depends on user-provided value" alerts. Investigated each: - install_locally() (/api/install) built a filesystem path from metadata.id without validating it at that point -- it was only implicitly safe because _generate_plugin_files() validates the same field (re-extracted independently) earlier in the same request. That's a real gap: reorder or change that earlier call and it's an exploitable path traversal / arbitrary file write. Fixed by validating plugin_id directly against _PLUGIN_ID_RE at the point the path is built, matching the pattern already used correctly in validate_id() and load_plugin(). - The other 10 flagged locations (serve_font's allowlist check, validate_id, load_plugin and its downstream reads) were already guarded by an explicit check earlier in the same function -- false positives from CodeQL not modeling those as sanitizers. Also fixed 2 of the 5 "stack trace exposed" warnings that were genuine: install_locally() and load_plugin() returned raw OSError/Exception text to the client in a 500 response; now logged server-side with a generic client-facing message. The other 3 (generate_zip/install_locally/ preview_code returning str(ValueError) from _generate_plugin_files) are deliberate, human-authored validation messages, not exception internals -- left as-is. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ --- web_interface/blueprints/composer.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/web_interface/blueprints/composer.py b/web_interface/blueprints/composer.py index 68642a84..9b3461f6 100644 --- a/web_interface/blueprints/composer.py +++ b/web_interface/blueprints/composer.py @@ -647,6 +647,13 @@ def install_locally(): return jsonify({'status': 'error', 'message': str(exc)}), 422 plugin_id = data.get('metadata', {}).get('id', '') + # _generate_plugin_files() above already validates metadata.id via this + # same regex before it will return, but that guarantee lives in a + # different function -- re-check here, at the point the path is actually + # built, so this route stays safe on its own if that call is ever + # reordered or changed. + if not _PLUGIN_ID_RE.match(plugin_id): + return jsonify({'status': 'error', 'message': 'Invalid plugin ID'}), 400 target = Path(composer_bp.plugins_dir) / plugin_id force = bool(data.get('_force', False)) @@ -665,7 +672,8 @@ def install_locally(): (target / filename).write_text(content, encoding='utf-8') _save_composer_state(target, data) except OSError as exc: - return jsonify({'status': 'error', 'message': f'Failed to write plugin files: {exc}'}), 500 + logger.error('Failed to write plugin files for %s: %s', plugin_id, exc) + return jsonify({'status': 'error', 'message': 'Failed to write plugin files'}), 500 # Trigger plugin discovery so it shows up in the Plugin Manager immediately if composer_bp.plugin_manager: @@ -778,7 +786,8 @@ def load_plugin(plugin_id): state = json.loads(state_path.read_text()) return jsonify({'status': 'ok', 'source': 'composer', 'state': state}) except Exception as exc: - return jsonify({'status': 'error', 'message': f'Failed to read state: {exc}'}), 500 + logger.error('Failed to read composer state for %s: %s', plugin_id, exc) + return jsonify({'status': 'error', 'message': 'Failed to read state'}), 500 # Partial import from config_schema.json schema_path = plugin_dir / 'config_schema.json'