mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-01 16:58:06 +00:00
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ
This commit is contained in:
co-authored by
Claude Sonnet 5
parent
47e3021fc3
commit
cd7e16e58e
@@ -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'
|
||||
|
||||
Reference in New Issue
Block a user