From 986f74e38bd7c12e2f2fd9934bb54ee3709ca4ca Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Fri, 21 Aug 2026 19:38:54 -0400 Subject: [PATCH] fix(composer): reject config keys that shadow plugin state Five review findings, plus the two bandit reported. Config variable keys were checked against an identifier regex only. Python keywords slipped past it and were caught downstream by ast.parse, but reported as Generated code has a syntax error: invalid syntax (, line 17) which names neither the field nor the value. They are now refused by name, soft keywords ('match', 'case') included. Worse, a key matching a BasePlugin attribute generated *valid* code that silently clobbered plugin state. 'config' is the sharp one: the assignment lands immediately after super().__init__(), so self.config = config.get("config", "x") replaces the plugin's config dict with a string, and every later self.config.get(...) fails at runtime. Refused now, along with logger, display_manager, cache_manager, plugin_id, enabled, self and the lifecycle method names. A test pins the ordering assumption that reserved list rests on, so it fails if config vars are ever emitted before super().__init__() instead. Also: - The silent `except Exception: pass` around manifest parsing now logs. It left "partial import produced nothing" indistinguishable from a malformed manifest. (bandit B110) - list_plugins() called iterdir() on a directory that may not exist -- a fresh install or a bad path returned 500 instead of an empty list. - metadata.id is stripped in the two route handlers, matching _generate_plugin_files, which strips before validating. Without it " my-plugin " generated fine and then failed the id check at install, reading as a generator bug. - The jinja Environment's autoescape=False now says why: these templates emit Python, and escaping a quote to " inside generated code would break it. Safety comes from the values instead -- _safe_int, _rgb_expr and _reject_source_breaking, all covered by the injection suite. (bandit B701, marked nosec with that rationale) bandit on composer.py: 2 findings -> 0. Verified: 156 tests across the two composer suites. Removing either new key check fails 9. Not reproduced: the suggestion to emit `pass` so a conditional block is never empty. 'line' and 'divider' render through a different template branch and 'section' emits nothing at all, so no element type available here produces an `if width >= N:` with an empty body. Left alone rather than changing template output speculatively. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --- test/test_composer_code_injection.py | 54 +++++++++++++++++++++++++++ web_interface/blueprints/composer.py | 56 +++++++++++++++++++++++++--- 2 files changed, 105 insertions(+), 5 deletions(-) diff --git a/test/test_composer_code_injection.py b/test/test_composer_code_injection.py index 542ed378..54a5b77b 100644 --- a/test/test_composer_code_injection.py +++ b/test/test_composer_code_injection.py @@ -115,3 +115,57 @@ def test_the_generated_module_still_has_no_top_level_statements(): src = _generated(_payload(elements=[el])) assert not _module_level_code(src) assert "(1, 2, 3)" in src + + +# --- config variable keys --------------------------------------------------- + +def _with_key(key): + return {"metadata": dict(BASE_META), "elements": [], + "dataModel": {"configVars": [{"key": key, "type": "string", + "default": "x", "label": "L"}]}} + + +@pytest.mark.parametrize("key", ["class", "def", "import", "None", "True", + "lambda", "pass", "match", "case"]) +def test_a_keyword_config_key_is_named_in_the_error(key): + """ast.parse already rejected these, but as an unhelpful line number. + + "Generated code has a syntax error: invalid syntax (line 17)" tells the + user nothing about which field to fix. + """ + with pytest.raises(C.ComposerInputError) as exc: + _generated(_with_key(key)) + assert key in str(exc.value) and "keyword" in str(exc.value).lower() + + +@pytest.mark.parametrize("key", ["config", "logger", "display_manager", + "cache_manager", "plugin_id", "enabled", + "self", "update", "display"]) +def test_a_reserved_attribute_config_key_is_refused(key): + """These generate *valid* Python that silently clobbers plugin state. + + The worst is `config`: the assignment lands right after super().__init__(), + so `self.config = config.get("config", "x")` replaces the plugin's config + dict with a string and every later self.config.get(...) fails at runtime. + """ + with pytest.raises(C.ComposerInputError) as exc: + _generated(_with_key(key)) + assert key in str(exc.value) and "reserved" in str(exc.value).lower() + + +@pytest.mark.parametrize("key", ["brightness", "my_var", "_private", "x1", + "update_interval_seconds"]) +def test_ordinary_config_keys_are_still_accepted(key): + src = _generated(_with_key(key)) + assert f"self.{key} = config.get(" in src + + +def test_the_generated_config_assignment_does_not_precede_super_init(): + """Guards the reasoning behind the reserved list, not just the list.""" + src = _generated(_with_key("brightness")) + body = src.splitlines() + super_at = next(i for i, l in enumerate(body) if "super().__init__(" in l) + assign_at = next(i for i, l in enumerate(body) if "self.brightness = config.get(" in l) + assert assign_at > super_at, ( + "config vars are assigned before super().__init__(); the reserved-name " + "list assumes they land after it") diff --git a/web_interface/blueprints/composer.py b/web_interface/blueprints/composer.py index 544aa086..010395df 100644 --- a/web_interface/blueprints/composer.py +++ b/web_interface/blueprints/composer.py @@ -11,6 +11,7 @@ Routes: import ast import io import json +import keyword import logging import os import re @@ -67,8 +68,16 @@ def _get_jinja_env() -> jinja2.Environment: global _jinja_env if _jinja_env is None: template_dir = Path(__file__).parent.parent / 'templates' / 'v3' / 'composer' - _jinja_env = jinja2.Environment( + _jinja_env = jinja2.Environment( # nosec B701 - see below loader=jinja2.FileSystemLoader(str(template_dir)), + # These templates emit Python source, not HTML. Autoescaping would + # turn a quote in a plugin name into " inside generated code + # and break it, so it stays off deliberately -- and the safety has + # to come from the values instead. It does: every numeric value is + # coerced by _safe_int/_rgb_expr, and text that could terminate a + # string literal is rejected by _reject_source_breaking. Both are + # covered by test/test_composer_code_injection.py, which is where + # to look before relaxing any of it. autoescape=False, trim_blocks=True, lstrip_blocks=True, @@ -101,6 +110,16 @@ def _to_class_name(name: str) -> str: return base if base.endswith('Plugin') else base + 'Plugin' +#: Attribute names BasePlugin (or the generated __init__) already owns. A +#: config var using one of these produces valid Python that quietly clobbers +#: the plugin's own state instead of failing loudly. +_RESERVED_ATTRS = frozenset({ + 'config', 'logger', 'display_manager', 'cache_manager', 'plugin_manager', + 'plugin_id', 'enabled', 'global_config', 'self', 'update', 'display', + 'validate_config', 'get_info', 'cleanup', +}) + + def _reject_source_breaking(value: str, field: str) -> None: """Refuse text that could terminate a string literal in generated source. @@ -518,6 +537,22 @@ def _generate_plugin_files(data: dict) -> dict: key = cv.get('key', '') if not _PYTHON_IDENT_RE.match(key): raise ComposerInputError(f'Config variable key "{key}" is not a valid Python identifier.') + # A keyword produces `self.class = ...`, which the ast.parse check + # below does catch -- but as "Generated code has a syntax error: + # invalid syntax (line 17)", which tells the user nothing about which + # field to fix. + if keyword.iskeyword(key) or keyword.issoftkeyword(key): + raise ComposerInputError( + f'Config variable key "{key}" is a Python keyword.') + # These generate *valid* code that silently shadows the plugin's own + # state. "config" is the worst: the assignment runs immediately after + # super().__init__(), so + # self.config = config.get("config", "x") + # replaces the plugin's config dict with a string and every later + # self.config.get(...) fails at runtime. + if key in _RESERVED_ATTRS: + raise ComposerInputError( + f'Config variable key "{key}" is reserved by BasePlugin.') class_name = _to_class_name(plugin_name) # Only consider visible elements for code generation flags @@ -742,7 +777,10 @@ def generate_zip(): logger.exception('Unexpected error generating plugin files: %s', exc) return jsonify({'status': 'error', 'message': 'Could not generate plugin files'}), 422 - plugin_id = data.get('metadata', {}).get('id', 'plugin') + # .strip() to match _generate_plugin_files, which strips before it + # validates. Without it " my-plugin " generates successfully and then + # fails the id check here, which reads as a bug in the generator. + plugin_id = data.get('metadata', {}).get('id', 'plugin').strip() or 'plugin' files['_composer_state.json'] = json.dumps(data, indent=2, ensure_ascii=False) zip_buf = _pack_zip(files, plugin_id) return send_file( @@ -772,7 +810,7 @@ def install_locally(): logger.exception('Unexpected error generating plugin files: %s', exc) return jsonify({'status': 'error', 'message': 'Could not generate plugin files'}), 422 - plugin_id = data.get('metadata', {}).get('id', '') + plugin_id = data.get('metadata', {}).get('id', '').strip() # _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 @@ -855,6 +893,12 @@ def list_plugins(): if not composer_bp.plugins_dir: return jsonify([]) plugins_dir = Path(composer_bp.plugins_dir) + if not plugins_dir.is_dir(): + # Configured but not created yet -- a fresh install, or a bad path. + # iterdir() raises FileNotFoundError/NotADirectoryError here, which + # surfaced as a 500 rather than "no plugins". + logger.warning("Plugin directory %s does not exist", plugins_dir) + return jsonify([]) results = [] for entry in sorted(plugins_dir.iterdir()): if not entry.is_dir(): @@ -964,8 +1008,10 @@ def load_plugin(plugin_id): if manifest_path.exists(): try: manifest = json.loads(manifest_path.read_text()) - except Exception: - pass + except (OSError, ValueError) as exc: + # Swallowing this left "partial import produced nothing" with no + # way to tell a malformed manifest from an absent one. + logger.warning("Failed to parse manifest.json for %s: %s", plugin_id, exc) partial_state = { 'composer_version': '1.0',