fix(composer): close binding_source code injection, line-align, and project_root UnboundLocalError

Three findings from CodeRabbit's review of 6c23994b, all verified against
current code before fixing:

- manager.py.j2 interpolated binding.source unescaped into a Python comment
  (`pass  # dynamic_text binding_source "{{ el.binding_source }}" draws
  nothing`). A source string with a newline broke out of the comment; a
  crafted payload produces a clean, ast.parse-valid `import os` in the
  generated plugin (confirmed against the pre-fix template). This is now a
  fixed literal comment that never interpolates the value. Live now that
  composer_bp is registered. CWE-94.
- _alignElement moved a line's x0 (or y0) to the new position but left x1
  (or y1) behind, so aligning a line changed its shape instead of moving
  it. Both endpoints now translate by the same delta.
- web_interface/app.py only assigned project_root inside the relative-path
  branch of the plugins_dir resolution. An absolute plugin_system.plugins_
  directory (a supported config value) hit UnboundLocalError importing the
  module at all, since SchemaManager/composer_bp use project_root further
  down. Now assigned unconditionally before the branch.

Also extends BOUND_TYPES coverage in composer-app.js (_isBound,
removeConfigVar, _validateBeforeExport) from dynamic_text/progress_bar to
all six element types that carry a binding object (countdown, pips,
sparkline, gauge too) -- found by direct code reading against
ELEMENT_DEFAULTS in composer-canvas.js, not from a review comment. Without
it, those four types could export with an unbound config key with no
validation error, and deleting a config var they used gave no warning.

All four fixes have mutation-checked regression tests (fail against the
reverted code, pass with the fix): test_binding_source_cannot_break_out_of_the_comment_it_lands_in,
test_align_translates_both_line_endpoints_not_just_the_start,
test_app_plugins_dir_resolution.py, test_binding_checks_cover_every_bound_element_type.

Full suite: 4365 passed, 58 skipped, 2 failed -- both the pre-existing
Europe/Kiev/Asia/Calcutta tzdata-alias gap on this sandbox, identical on
origin/main, unrelated to this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

test/test_app_plugins_dir_resolution.py

@@ -0,0 +1,52 @@
+"""web_interface/app.py resolves plugins_dir from config at import time.
+
+`project_root` used to be assigned only inside the `else` branch (the
+relative-path case). SchemaManager and composer_bp are wired up with
+`project_root` further down the same module, so a device configured with an
+*absolute* `plugin_system.plugins_directory` (a supported value -- see
+CLAUDE.md) hit `UnboundLocalError` importing web_interface.app at all, taking
+the whole web UI down.
+
+app.py isn't imported directly by the test suite -- it has real side effects
+(managers, background state) at import time -- so this extracts the exact
+resolution block by its stable comment markers and executes it in isolation,
+the same way the rest of the suite validates generated/templated code rather
+than re-implementing it.
+"""
+from pathlib import Path
+
+APP_PY = Path(__file__).resolve().parent.parent / "web_interface/app.py"
+
+START_MARKER = "# Resolve plugin directory - handle both absolute and relative paths\n"
+END_MARKER = "\nplugin_manager = PluginManager("
+
+
+def _resolution_block() -> str:
+    src = APP_PY.read_text()
+    start = src.index(START_MARKER)
+    end = src.index(END_MARKER, start)
+    return src[start:end]
+
+
+def test_resolution_block_still_matches_expected_markers():
+    """If app.py is restructured enough that these markers move, the exec
+    below would silently test nothing -- fail loudly instead."""
+    block = _resolution_block()
+    assert "plugins_dir" in block and "os.path.isabs" in block
+
+
+def test_absolute_plugins_directory_does_not_raise_unboundlocalerror():
+    import os
+    ns = {"os": os, "Path": Path, "__file__": str(APP_PY),
+          "plugins_dir_name": "/opt/ledmatrix-plugins"}
+    exec(compile(_resolution_block(), str(APP_PY), "exec"), ns)  # noqa: S102
+    assert ns["project_root"] == APP_PY.parent.parent
+    assert ns["plugins_dir"] == Path("/opt/ledmatrix-plugins")
+
+
+def test_relative_plugins_directory_still_resolves_under_project_root():
+    import os
+    ns = {"os": os, "Path": Path, "__file__": str(APP_PY),
+          "plugins_dir_name": "plugin-repos"}
+    exec(compile(_resolution_block(), str(APP_PY), "exec"), ns)  # noqa: S102
+    assert ns["plugins_dir"] == ns["project_root"] / "plugin-repos"

test/test_composer_empty_block.py

@@ -17,6 +17,7 @@ has no branch for, and the template emits a `pass` fallback so a type added to
 the canvas before its branch exists degrades to a no-op instead of a broken
 plugin.
 """
+import ast
 import re
 import sys
 from pathlib import Path
@@ -98,4 +99,26 @@ def test_dynamic_text_with_non_config_binding_does_not_break_generation(wrapper)
                "binding": {"source": "live", "key": "temperature"}, **wrapper}
     files = generate(element)          # must not raise ComposerInputError
     assert "manager.py" in files
-    assert 'binding_source "live" draws nothing' in files["manager.py"]
+    assert "non-config dynamic_text binding draws nothing" in files["manager.py"]
+
+
+def test_binding_source_cannot_break_out_of_the_comment_it_lands_in():
+    """binding.source used to be interpolated straight into a Python comment
+    (`pass  # dynamic_text binding_source "{{ el.binding_source }}" draws
+    nothing`) with no escaping. A source string carrying a newline closed the
+    comment, and text on the following line(s), indented to match, became a
+    real statement in the generated plugin -- CWE-94, and live once
+    composer_bp was registered (confirmed: this exact payload produces a
+    manager.py containing a clean, ast.parse-valid `import os` against the
+    pre-fix template). The comment is now a fixed literal that never
+    interpolates the value at all.
+    """
+    payload = "foo\n            import os\n            os.system('id')  # "
+    element = {"type": "dynamic_text", "x": 0, "y": 0, "color": "#ffffff",
+               "binding": {"source": payload, "key": "temperature"}}
+    files = generate(element)
+    src = files["manager.py"]
+    assert "import os" not in src
+    assert "os.system" not in src
+    assert "non-config dynamic_text binding draws nothing" in src
+    ast.parse(src)          # belt and braces: generate() already enforces this

test/test_composer_js_contracts.py

@@ -187,6 +187,20 @@ def test_align_clears_the_anchor_and_moves_line_endpoints():
         "_alignElement no longer moves line endpoints"
 
 
+def test_align_translates_both_line_endpoints_not_just_the_start():
+    """Setting only x0 (or y0) left x1/y1 behind, so aligning a line changed
+    its shape instead of moving it -- e.g. a line from x0=20 to x1=50 aligned
+    right became x0=98, x1=50, stretching rather than translating it. Both
+    endpoints must move by the same delta."""
+    body = _method_source(APP, "_alignElement")
+    x_branch = body[body.index("if (axis === 'x')"):body.index("} else {")]
+    y_branch = body[body.index("} else {"):]
+    assert "el.x1" in x_branch, \
+        "_alignElement moves x0 but not x1 -- a line's shape changes, not its position"
+    assert "el.y1" in y_branch, \
+        "_alignElement moves y0 but not y1 -- a line's shape changes, not its position"
+
+
 def test_divider_stroke_is_centered_in_led_pixels():
     """A 0.5 canvas-pixel offset (correct only at SCALE=1) was applied after
     scaling instead of before it, so at SCALE>1 the stroke bled into the
@@ -215,3 +229,35 @@ def test_gauge_radii_are_clamped_to_zero():
         "gauge y-radius is not clamped to zero"
     assert "rx - lwPx / 2" not in re.sub(r"Math\.max\(0,\s*rx\s*-\s*lwPx\s*/\s*2\)", "", gauge_branch), \
         "an unclamped gauge radius is still passed to ctx.ellipse()"
+
+
+#: ELEMENT_DEFAULTS in composer-canvas.js gives exactly these types a
+#: `binding` object; verified against that file rather than assumed.
+BOUND_TYPES = ["dynamic_text", "progress_bar", "countdown", "pips", "sparkline", "gauge"]
+
+
+def test_bound_types_constant_lists_every_bound_element_type():
+    """ELEMENT_DEFAULTS in composer-canvas.js gives dynamic_text, progress_bar,
+    countdown, pips, sparkline and gauge a `binding` object. If a type is added
+    to (or removed from) that list without updating APP's BOUND_TYPES, the
+    checks below drift out of sync silently -- this pins the two together."""
+    src = APP.read_text()
+    match = re.search(r"const BOUND_TYPES = \[([^\]]*)\];", src)
+    assert match, "BOUND_TYPES constant not found in composer-app.js"
+    declared = {t.strip().strip("'\"") for t in match.group(1).split(",") if t.strip()}
+    assert declared == set(BOUND_TYPES), \
+        f"BOUND_TYPES {declared} does not match the bound element types {set(BOUND_TYPES)}"
+
+
+@pytest.mark.parametrize("method", ["_isBound", "removeConfigVar", "_validateBeforeExport"])
+def test_binding_checks_cover_every_bound_element_type(method):
+    """Only dynamic_text and progress_bar were checked, so a countdown, pips,
+    sparkline or gauge element with an empty binding passed export validation
+    silently, and deleting a config var still used by one of them gave no
+    warning -- the plugin would read a now-missing key at runtime with no
+    indication why."""
+    body = _method_source(APP, method)
+    assert "BOUND_TYPES.includes(e.type)" in body, \
+        f"{method} does not check every bound element type via BOUND_TYPES"
+    assert "e.type === 'dynamic_text'" not in body, \
+        f"{method} still hardcodes only dynamic_text instead of BOUND_TYPES"

web_interface/app.py

@@ -83,11 +83,11 @@ plugin_system_config = config.get('plugin_system', {})
 plugins_dir_name = plugin_system_config.get('plugins_directory', 'plugin-repos')
 
 # Resolve plugin directory - handle both absolute and relative paths
+project_root = Path(__file__).parent.parent
 if os.path.isabs(plugins_dir_name):
     plugins_dir = Path(plugins_dir_name)
 else:
     # If relative, resolve relative to the project root (LEDMatrix directory)
-    project_root = Path(__file__).parent.parent
     plugins_dir = project_root / plugins_dir_name
 
 plugin_manager = PluginManager(

web_interface/static/v3/js/composer/composer-app.js

@@ -182,6 +182,14 @@ const LED_PALETTE = [
 
 // ── Autosave helpers ─────────────────────────────────────────────────────────
 const LS_KEY = 'ledmatrix_composer_draft';
+
+//: Element types whose ELEMENT_DEFAULTS carry a `binding` object (see
+//: composer-canvas.js). Kept in one place so export validation, the "is this
+//: config var still used" check, and the removal warning cannot drift apart
+//: the way they did when only dynamic_text/progress_bar were checked and
+//: countdown/pips/sparkline/gauge silently went unvalidated.
+const BOUND_TYPES = ['dynamic_text', 'progress_bar', 'countdown', 'pips', 'sparkline', 'gauge'];
+
 let _autosaveTimer = null;
 
 function _debouncedAutosave(payload) {
@@ -867,15 +875,30 @@ function composerApp() {
                    :                    this.MATRIX_W - bb.w;
         // Clear x-anchor so stored x IS the absolute position
         if ('xAnchor' in el) el.xAnchor = null;
-        el.x = Math.round(newX);
-        if (el.type === 'line') el.x0 = Math.round(newX);
+        if (el.type === 'line') {
+          // Translate both endpoints by the same delta so the line moves
+          // without changing shape -- setting only x0 left x1 behind and
+          // stretched/shrank the line instead of moving it.
+          const dx = Math.round(newX) - bb.x;
+          el.x0 = Math.round(el.x0 + dx);
+          el.x1 = Math.round(el.x1 + dx);
+          el.x = el.x0;
+        } else {
+          el.x = Math.round(newX);
+        }
       } else {
         const newY = mode === 'start'  ? 0
                    : mode === 'center' ? Math.round((this.MATRIX_H - bb.h) / 2)
                    :                    this.MATRIX_H - bb.h;
         if ('yAnchor' in el) el.yAnchor = null;
-        el.y = Math.round(newY);
-        if (el.type === 'line') el.y0 = Math.round(newY);
+        if (el.type === 'line') {
+          const dy = Math.round(newY) - bb.y;
+          el.y0 = Math.round(el.y0 + dy);
+          el.y1 = Math.round(el.y1 + dy);
+          el.y = el.y0;
+        } else {
+          el.y = Math.round(newY);
+        }
       }
       this._snapshot();
       this.isDirty = true;
@@ -1162,7 +1185,7 @@ function composerApp() {
 
     removeConfigVar(key) {
       const bound = this.elements.filter(
-        e => e.type === 'dynamic_text' && e.binding?.source === 'config' && e.binding?.key === key
+        e => BOUND_TYPES.includes(e.type) && e.binding?.source === 'config' && e.binding?.key === key
       );
       if (bound.length && !confirm(`"${key}" is used by ${bound.length} element(s). Remove anyway?`)) return;
       this.dataModel.configVars = this.dataModel.configVars.filter(v => v.key !== key);
@@ -1172,7 +1195,7 @@ function composerApp() {
 
     _isBound(key) {
       return this.elements.some(
-        e => e.type === 'dynamic_text' && e.binding?.source === 'config' && e.binding?.key === key
+        e => BOUND_TYPES.includes(e.type) && e.binding?.source === 'config' && e.binding?.key === key
       );
     },
 
@@ -1306,7 +1329,7 @@ function composerApp() {
       if (!this.metadata.author.trim()) { this._setStatus('Author is required', 'error'); return false; }
       if (this.elements.length === 0)   { this._setStatus('Add at least one element', 'error'); return false; }
       const unbound = this.elements.filter(
-        e => (e.type === 'dynamic_text' || e.type === 'progress_bar') && !e.binding?.key
+        e => BOUND_TYPES.includes(e.type) && !e.binding?.key
       );
       if (unbound.length) { this._setStatus(`${unbound.length} element(s) have no variable bound`, 'error'); return false; }
       return true;

web_interface/templates/v3/composer/manager.py.j2

@@ -111,7 +111,7 @@ class {{ class_name }}(BasePlugin):
 {{ pi }}    font=self.display_manager.{{ el.font_attr }},
 {{ pi }})
 {% else %}
-{{ pi }}pass  # dynamic_text binding_source "{{ el.binding_source }}" draws nothing
+{{ pi }}pass  # non-config dynamic_text binding draws nothing
 {% endif %}
 {% elif el.type == 'clock' %}
 {{ pi }}self.display_manager.draw_text(