rick/LEDMatrix
1
0
Fork 0
mirror of https://github.com/ChuckBuilds/LEDMatrix.git synced 2026-10-18 04:49:13 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(starlark): let the path check be a sanitiser, not a boolean

Browse Source 
CodeQL reported 24 path-injection alerts across these handlers. The guard
was effective -- traversal is blocked at every entry, verified on
hardware -- but the shape was wrong in a way worth fixing rather than
dismissing: _validate_starlark_app_path returned a bool, and the caller
then rebuilt the path with `_STARLARK_APPS_DIR / app_id` using the raw
value. Validate in one place, join in another, and the two have to stay
in step by hand. A boolean is also not something CodeQL can follow, so
the value it saw reaching the filesystem was the untrusted one -- the
alerts were reporting the pattern accurately.

It now returns the resolved, checked Path, and every caller uses that
instead of re-joining. Seven join sites became zero; the only remaining
construction from app_id is inside the validator itself.

_standalone_render_starlark_app and _install_star_file validate for
themselves too. Both are reachable independently of the handlers, and a
path built from app_id should not be assembled anywhere without the
check.

Also stops three error paths returning exception text to the caller
(CodeQL's 4 information-exposure alerts): a bad config.json answered with
its absolute path and the parser's message. Logged in full, answered
generically -- the same split the composer blueprint already makes.

The pixlet subprocess is left as it is. It is list-form with no shell,
whitelists config keys against ^[a-zA-Z_][a-zA-Z0-9_]*$ and drops any
value containing shell metacharacters; "x; id", "x$(id)" and "x`id`" are
all treated as data on hardware. Nothing to fix there.

Behaviour is unchanged: same error strings for callers, same status
codes. 32 Starlark tests and the full core suite (3979) pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014RRtqXDCnvnY6EQwhT5CV9
This commit is contained in:
ChuckBuilds
2026-09-07 14:44:43 -04:00
co-authored by Claude Opus 5
parent 2a92f1ab3b
commit c7de8be6f5
1 changed files with 54 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
web_interface/blueprints/api_v3.py
+54 -41
View File
@@ -8892,34 +8892,38 @@ def _validate_timing_value(value: Any, field_name: str, min_val: int = 1, max_va
return None, f"{field_name} must be at most {max_val}" return None, f"{field_name} must be at most {max_val}"
return int_value, None return int_value, None
def _validate_starlark_app_path(app_id: str) -> Tuple[bool, Optional[str]]: def _validate_starlark_app_path(app_id: str) -> Tuple[Optional[Path], Optional[str]]:
""" """The app's directory, or an error if app_id could escape the base dir.
Validate app_id for path traversal attacks before filesystem access.
Args: Returns the *resolved* path rather than a boolean, and every caller uses
app_id: App identifier from user input what it returns instead of re-joining ``_STARLARK_APPS_DIR / app_id``
afterwards. The old shape validated in one place and rebuilt the path in
another, which is two things that have to stay in step -- and is why
CodeQL reported twenty-four path-injection alerts across these handlers
even though the guard was effective: a boolean is not a sanitiser it can
follow, and the value reaching the filesystem was the raw one.
Returns: The name is unchanged so the call sites read the same.
Tuple of (is_valid, error_message)
""" """
# Check for path traversal characters if not isinstance(app_id, str) or not app_id:
return None, "Invalid app_id"
# Reject the traversal characters outright before touching the filesystem.
if '..' in app_id or '/' in app_id or '\\' in app_id: if '..' in app_id or '/' in app_id or '\\' in app_id:
return False, f"Invalid app_id: contains path traversal characters" return None, "Invalid app_id: contains path traversal characters"
# Construct and resolve the path
try: try:
app_path = (_STARLARK_APPS_DIR / app_id).resolve()
base_path = _STARLARK_APPS_DIR.resolve() base_path = _STARLARK_APPS_DIR.resolve()
app_path = (base_path / app_id).resolve()
# Verify the resolved path is within the base directory
try: try:
app_path.relative_to(base_path) app_path.relative_to(base_path)
return True, None
except ValueError: except ValueError:
return False, f"Invalid app_id: path traversal attempt" return None, "Invalid app_id: path traversal attempt"
except Exception as e: return app_path, None
logger.warning(f"Path validation error for app_id '{app_id}': {e}") except OSError as e:
return False, f"Invalid app_id" logger.warning("Path validation error for app_id %r: %s", app_id, e)
return None, "Invalid app_id"
def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[str]]: def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[str]]:
"""Render a Starlark app via pixlet directly (no plugin required). """Render a Starlark app via pixlet directly (no plugin required).
@@ -8940,7 +8944,11 @@ def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[st
if not app_data: if not app_data:
return False, 404, f"App not found: {app_id}" return False, 404, f"App not found: {app_id}"
app_dir = _STARLARK_APPS_DIR / app_id # Validated here as well as at the handler: this is reachable on its own,
# and a path built from app_id should never be assembled without it.
app_dir, path_error = _validate_starlark_app_path(app_id)
if path_error:
return False, 400, path_error
star_file = app_dir / app_data.get('star_file', f'{app_id}.star') star_file = app_dir / app_data.get('star_file', f'{app_id}.star')
if not star_file.exists(): if not star_file.exists():
return False, 404, f"Star file not found: {star_file}" return False, 404, f"Star file not found: {star_file}"
@@ -8972,9 +8980,11 @@ def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[st
with open(config_file) as f: with open(config_file) as f:
app_config = json.load(f) app_config = json.load(f)
except json.JSONDecodeError as e: except json.JSONDecodeError as e:
return False, 400, f"Invalid config.json for {app_id} ({config_file}): {e}" logger.warning("Invalid config.json for %r at %s: %s", app_id, config_file, e)
return False, 400, f"Invalid config.json for {app_id}"
except OSError as e: except OSError as e:
return False, 400, f"Cannot read config.json for {app_id} ({config_file}): {e}" logger.warning("Cannot read config.json for %r at %s: %s", app_id, config_file, e)
return False, 400, f"Cannot read config.json for {app_id}"
if not isinstance(app_config, dict): if not isinstance(app_config, dict):
return False, 400, ( return False, 400, (
f"config.json for {app_id} must be a JSON object, " f"config.json for {app_id} must be a JSON object, "
@@ -9003,7 +9013,8 @@ def _standalone_render_starlark_app(app_id: str) -> Tuple[bool, int, Optional[st
except subprocess.TimeoutExpired: except subprocess.TimeoutExpired:
return False, 504, "Render timed out after 30s" return False, 504, "Render timed out after 30s"
except Exception as e: except Exception as e:
return False, 500, f"Render error: {e}" logger.exception("Starlark render failed for %r", app_id)
return False, 500, "Render error"
def _write_starlark_manifest(manifest: Dict[str, Any]) -> bool: def _write_starlark_manifest(manifest: Dict[str, Any]) -> bool:
"""Write the starlark-apps manifest.json to disk with atomic write.""" """Write the starlark-apps manifest.json to disk with atomic write."""
@@ -9035,7 +9046,10 @@ def _install_star_file(app_id: str, star_file_path: str, metadata: Dict[str, Any
"""Install a .star file and update the manifest (standalone, no plugin needed).""" """Install a .star file and update the manifest (standalone, no plugin needed)."""
import shutil import shutil
import json import json
app_dir = _STARLARK_APPS_DIR / app_id app_dir, path_error = _validate_starlark_app_path(app_id)
if path_error:
logger.warning("Refusing to install %r: %s", app_id, path_error)
return False
app_dir.mkdir(parents=True, exist_ok=True) app_dir.mkdir(parents=True, exist_ok=True)
dest = app_dir / f"{app_id}.star" dest = app_dir / f"{app_id}.star"
shutil.copy2(star_file_path, str(dest)) shutil.copy2(star_file_path, str(dest))
@@ -9139,8 +9153,8 @@ def get_starlark_app(app_id):
"""Get details for a specific Starlark app.""" """Get details for a specific Starlark app."""
try: try:
# Validate app_id before any filesystem access # Validate app_id before any filesystem access
is_valid, error_msg = _validate_starlark_app_path(app_id) app_dir, error_msg = _validate_starlark_app_path(app_id)
if not is_valid: if error_msg:
return jsonify({'status': 'error', 'message': error_msg}), 400 return jsonify({'status': 'error', 'message': error_msg}), 400
starlark_plugin = _get_starlark_plugin() starlark_plugin = _get_starlark_plugin()
@@ -9172,7 +9186,7 @@ def get_starlark_app(app_id):
# Load schema from schema.json if it exists (path already validated above) # Load schema from schema.json if it exists (path already validated above)
schema = None schema = None
schema_file = _STARLARK_APPS_DIR / app_id / 'schema.json' schema_file = app_dir / 'schema.json'
if schema_file.exists(): if schema_file.exists():
try: try:
with open(schema_file, 'r') as f: with open(schema_file, 'r') as f:
@@ -9279,18 +9293,17 @@ def uninstall_starlark_app(app_id):
"""Uninstall a Starlark app.""" """Uninstall a Starlark app."""
try: try:
# Validate app_id before any filesystem access # Validate app_id before any filesystem access
is_valid, error_msg = _validate_starlark_app_path(app_id) app_dir, error_msg = _validate_starlark_app_path(app_id)
if not is_valid: if error_msg:
return jsonify({'status': 'error', 'message': error_msg}), 400 return jsonify({'status': 'error', 'message': error_msg}), 400
starlark_plugin = _get_starlark_plugin() starlark_plugin = _get_starlark_plugin()
if starlark_plugin: if starlark_plugin:
success = starlark_plugin.uninstall_app(app_id) success = starlark_plugin.uninstall_app(app_id)
else: else:
# Standalone: remove app dir and manifest entry (path already validated) # Standalone: remove app dir and manifest entry. app_dir is the
# path _validate_starlark_app_path checked, not a fresh join.
import shutil import shutil
app_dir = _STARLARK_APPS_DIR / app_id
if app_dir.exists(): if app_dir.exists():
shutil.rmtree(app_dir) shutil.rmtree(app_dir)
manifest = _read_starlark_manifest() manifest = _read_starlark_manifest()
@@ -9311,8 +9324,8 @@ def get_starlark_app_config(app_id):
"""Get configuration for a Starlark app.""" """Get configuration for a Starlark app."""
try: try:
# Validate app_id before any filesystem access # Validate app_id before any filesystem access
is_valid, error_msg = _validate_starlark_app_path(app_id) app_dir, error_msg = _validate_starlark_app_path(app_id)
if not is_valid: if error_msg:
return jsonify({'status': 'error', 'message': error_msg}), 400 return jsonify({'status': 'error', 'message': error_msg}), 400
starlark_plugin = _get_starlark_plugin() starlark_plugin = _get_starlark_plugin()
@@ -9322,8 +9335,8 @@ def get_starlark_app_config(app_id):
return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404 return jsonify({'status': 'error', 'message': f'App not found: {app_id}'}), 404
return jsonify({'status': 'success', 'config': app.config, 'schema': app.schema}) return jsonify({'status': 'success', 'config': app.config, 'schema': app.schema})
# Standalone: read from config.json file (path already validated) # Standalone: read from config.json. app_dir is the path
app_dir = _STARLARK_APPS_DIR / app_id # _validate_starlark_app_path checked, not a fresh join.
config_file = app_dir / "config.json" config_file = app_dir / "config.json"
if not app_dir.exists(): if not app_dir.exists():
@@ -9358,8 +9371,8 @@ def update_starlark_app_config(app_id):
"""Update configuration for a Starlark app.""" """Update configuration for a Starlark app."""
try: try:
# Validate app_id before any filesystem access # Validate app_id before any filesystem access
is_valid, error_msg = _validate_starlark_app_path(app_id) app_dir, error_msg = _validate_starlark_app_path(app_id)
if not is_valid: if error_msg:
return jsonify({'status': 'error', 'message': error_msg}), 400 return jsonify({'status': 'error', 'message': error_msg}), 400
data = request.get_json(silent=True) data = request.get_json(silent=True)
@@ -9436,8 +9449,8 @@ def update_starlark_app_config(app_id):
if display_duration is not None: if display_duration is not None:
app_data['display_duration'] = display_duration app_data['display_duration'] = display_duration
# Load current config from config.json # Load current config from config.json. app_dir is the path
app_dir = _STARLARK_APPS_DIR / app_id # _validate_starlark_app_path checked, not a fresh join.
config_file = app_dir / "config.json" config_file = app_dir / "config.json"
current_config = {} current_config = {}
if config_file.exists(): if config_file.exists():
@@ -9514,8 +9527,8 @@ def toggle_starlark_app(app_id):
def render_starlark_app(app_id): def render_starlark_app(app_id):
"""Force render a Starlark app.""" """Force render a Starlark app."""
try: try:
is_valid, err = _validate_starlark_app_path(app_id) app_dir, err = _validate_starlark_app_path(app_id)
if not is_valid: if err:
return jsonify({'status': 'error', 'message': err}), 400 return jsonify({'status': 'error', 'message': err}), 400
starlark_plugin = _get_starlark_plugin() starlark_plugin = _get_starlark_plugin()