diff --git a/test/test_app_plugins_dir_resolution.py b/test/test_app_plugins_dir_resolution.py new file mode 100644 index 00000000..521acefb --- /dev/null +++ b/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" diff --git a/test/test_composer_empty_block.py b/test/test_composer_empty_block.py index 296db78f..b5f2ba7e 100644 --- a/test/test_composer_empty_block.py +++ b/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 diff --git a/test/test_composer_js_contracts.py b/test/test_composer_js_contracts.py index a4525f4c..bb776c47 100644 --- a/test/test_composer_js_contracts.py +++ b/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" diff --git a/web_interface/app.py b/web_interface/app.py index 3adc2402..1922c6ca 100644 --- a/web_interface/app.py +++ b/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( diff --git a/web_interface/static/v3/js/composer/composer-app.js b/web_interface/static/v3/js/composer/composer-app.js index 7dd2230a..0e035cc9 100644 --- a/web_interface/static/v3/js/composer/composer-app.js +++ b/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; diff --git a/web_interface/templates/v3/composer/manager.py.j2 b/web_interface/templates/v3/composer/manager.py.j2 index c4ff2541..4dd771c2 100644 --- a/web_interface/templates/v3/composer/manager.py.j2 +++ b/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(