fix(composer): coerce prefixed colour channels, non-finite numbers, marquee ids

Three more routes into the generated source, plus a fix to one of my own tests
that was checking the wrong branch.

Prefixed colour channels were interpolated raw
----------------------------------------------
Five tuples were built without coercion:

    p['fill_tuple']  = f"({el.get('r', 100)}, {el.get('g', 200)}, ...)"
    p['empty_tuple'] = f"({el.get('emptyR', 50)}, ...)"
    p['label_tuple'] = f"({el.get('labelR', 200)}, ...)"

so progress_bar, pips, sparkline and gauge took arbitrary expressions the same
way width/height did. Confirmed: every one of the five put __import__ into the
generated source. They now go through a new _rgb_tuple helper, which _rgb_expr
also delegates to.

The pre-existing colour test only covered r/g/b on a text element, which is why
the prefixed channels and these four types were never exercised.

Non-finite numbers escaped as a 500
-----------------------------------
json.loads accepts Infinity/-Infinity/NaN by default and Flask's get_json
passes them straight through, so a payload can hand _safe_int a non-finite
float. int(inf) raises OverflowError, which is neither ValueError nor
ComposerInputError, so it escaped both handlers and surfaced as a 500 with a
traceback rather than a 422. Verified end to end through Flask's parser.

Marquee ids reached the source as identifiers
---------------------------------------------
data_key is spliced UNQUOTED into variable names (_{{ data_key }}_text = ...)
and only '-' was normalised. A punctuated id landed in the generated source as
code. ast.parse caught it, so this was not exploitable, but the caller got an
opaque "Generated code has a syntax error" instead of being told the id was
unusable -- the same failure mode as the empty-block bug. Now restricted to
identifier characters and bounded to 64.

The line-anchor test was testing the wrong branch
-------------------------------------------------
test_line_branch_applies_the_anchor_offset searched the whole file for
"case 'line': {". getBoundingBox has one too and comes first, so the assertion
was reading the bounding-box branch: stripping the anchor offset from
_drawElement left all 11 checks green. Both line tests are now scoped to their
own function via tree-sitter, so they cannot be satisfied by the same branch.

Tests: 35 of the injection suite's checks fail against the reverted fixes; the
scoped line test fails when _drawElement's offset is removed. Full suite 4059
passed, the one failure being test_install_lowmem (pre-existing, awaiting #492).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW

test/test_composer_code_injection.py

@@ -17,6 +17,7 @@ Two ways in, both confirmed against the code before it was fixed:
       -> f-string interpolated it verbatim: x=0 or __import__("os").system("id")
 """
 import ast
+import re
 import sys
 from pathlib import Path
 
@@ -48,7 +49,10 @@ LITERAL_PAYLOADS = [
 
 
 def _payload(**over):
-    p = {"metadata": dict(BASE_META), "elements": [], "config_vars": []}
+    # dataModel.configVars is the key _generate_plugin_files reads; "config_vars"
+    # was never looked at, so anything passed through it tested nothing.
+    p = {"metadata": dict(BASE_META), "elements": [],
+         "dataModel": {"configVars": over.pop("config_vars", [])}}
     p["metadata"].update(over.pop("metadata", {}))
     p.update(over)
     return p
@@ -77,17 +81,50 @@ def test_a_name_that_breaks_out_of_a_literal_is_refused(payload):
         _generated(_payload(metadata={"name": payload}))
 
 
+#: Types with a drawing branch in manager.py.j2. An injection test using any
+#: other type proves nothing: _preprocess_elements drops it, so its values
+#: never reach the generated source and every assertion passes trivially.
+#: This test previously used "line", which has never had a branch.
+RENDERED_GEOMETRY_CASES = [
+    ("rectangle", {"x": 0, "y": 0, "width": 10, "height": 8}),
+    ("arc", {"x": 0, "y": 0, "width": 24, "height": 24}),
+    ("ellipse", {"x": 0, "y": 0, "width": 24, "height": 12}),
+    ("rounded_rectangle", {"x": 0, "y": 0, "width": 24, "height": 10}),
+    ("gauge", {"x": 0, "y": 0, "width": 32, "height": 32}),
+]
+
+
+@pytest.mark.parametrize("etype,base", RENDERED_GEOMETRY_CASES)
 @pytest.mark.parametrize("evil", EXPR_PAYLOADS)
-@pytest.mark.parametrize("field", ["x", "y", "x0", "y0", "x1", "y1", "lineWidth"])
+@pytest.mark.parametrize("field", ["x", "y", "width", "height"])
-def test_a_non_numeric_geometry_value_cannot_reach_the_source(evil, field):
+def test_a_non_numeric_geometry_value_cannot_reach_the_source(etype, base, evil, field):
-    el = {"type": "line", "id": "l1", "x0": 0, "y0": 0, "x1": 10, "y1": 10,
+    """width/height were interpolated raw into the generated source.
-          "anchor_x": "right", "anchor_y": "bottom"}
+
+    p['x2_expr'] = f"({x_expr}) + {w}" with w straight off the payload, so a
+    rectangle with width='0 or __import__("os").system("id")' produced
+
+        [0, 0, (0) + 0 or __import__("os").system("id"), (0) + 8],
+
+    in a manager.py that /api/install writes to disk and the loader imports.
+    """
+    el = {"type": etype, "id": "e1", **base}
     el[field] = evil
     src = _generated(_payload(elements=[el]))
-    assert "__import__" not in src, f"{field}={evil!r} reached the generated source"
+    assert "__import__" not in src, f"{etype}.{field}={evil!r} reached the generated source"
     assert "os.system" not in src
     assert not _module_level_code(src), \
-        f"{field}={evil!r} produced module-level statements: {_module_level_code(src)}"
+        f"{etype}.{field}={evil!r} produced module-level statements: {_module_level_code(src)}"
+
+
+def test_every_injection_case_uses_a_type_that_actually_renders():
+    """Guards against the whole suite quietly going vacuous again.
+
+    An element type with no template branch is dropped before generation, so
+    an injection test written against one asserts nothing and still passes.
+    """
+    used = {etype for etype, _ in RENDERED_GEOMETRY_CASES}
+    missing = used - set(C._RENDERABLE_ELEMENT_TYPES)
+    assert not missing, f"injection tests use non-rendering types: {sorted(missing)}"
 
 
 @pytest.mark.parametrize("evil", EXPR_PAYLOADS)
@@ -195,3 +232,70 @@ def test_a_clock_without_a_format_uses_the_documented_default():
     el = {"type": "clock", "id": "c1", "x": 0, "y": 0, "font": "press_start"}
     src = _generated(_payload(elements=[el]))
     assert '"%H:%M"' in src, "the %H:%M default did not reach the generated source"
+
+
+#: (element type, channel key, base element) for colour channels that were
+#: interpolated raw rather than through _rgb_expr/_safe_int. Prefixed channels
+#: (emptyR/G/B, labelR/G/B) were the ones the original r/g/b test never reached.
+RAW_COLOUR_CASES = [
+    ("progress_bar", "r",      {"x": 0, "y": 0}),
+    ("progress_bar", "g",      {"x": 0, "y": 0}),
+    ("pips",         "b",      {"x": 0, "y": 0}),
+    ("pips",         "emptyR", {"x": 0, "y": 0}),
+    ("pips",         "emptyG", {"x": 0, "y": 0}),
+    ("sparkline",    "r",      {"x": 0, "y": 0}),
+    ("gauge",        "labelR", {"x": 0, "y": 0, "width": 32, "height": 32}),
+    ("gauge",        "labelB", {"x": 0, "y": 0, "width": 32, "height": 32}),
+]
+
+
+@pytest.mark.parametrize("etype,channel,base", RAW_COLOUR_CASES)
+@pytest.mark.parametrize("evil", EXPR_PAYLOADS)
+def test_a_prefixed_colour_channel_cannot_reach_the_source(etype, channel, base, evil):
+    """Five tuples were built with f"({el.get('r', 100)}, ...)" -- no coercion.
+
+    The pre-existing colour test only covered r/g/b on a text element, so the
+    prefixed channels and the four other types were never exercised.
+    """
+    el = {"type": etype, "id": "e1", **base}
+    el[channel] = evil
+    src = _generated(_payload(elements=[el]))
+    assert "__import__" not in src, f"{etype}.{channel}={evil!r} reached the source"
+    assert "os.system" not in src
+    assert not _module_level_code(src)
+
+
+@pytest.mark.parametrize("value", [float("inf"), float("-inf"), float("nan")])
+@pytest.mark.parametrize("field", ["x", "y", "width", "height"])
+def test_a_non_finite_dimension_does_not_escape_as_an_unhandled_error(field, value):
+    """json.loads accepts Infinity/NaN and Flask passes them through, so a
+    payload can hand _safe_int a non-finite float. int(inf) raises
+    OverflowError -- neither ValueError nor ComposerInputError -- so it escaped
+    both handlers and surfaced as a 500 with a traceback instead of a 422."""
+    el = {"type": "rectangle", "id": "r1", "x": 0, "y": 0, "width": 10, "height": 8}
+    el[field] = value
+    src = _generated(_payload(elements=[el]))       # must not raise
+    # A non-finite value must be replaced by the default, not spelled into the
+    # source. Word-boundary match: "info" in self.logger.info contains "inf".
+    assert not re.search(r"\b(inf|nan|Infinity|NaN)\b", src), \
+        f"{field}={value!r} leaked a non-finite literal into the source"
+    assert not _module_level_code(src)
+
+
+@pytest.mark.parametrize("bad_id", [
+    'x = __import__("os").system("id") #',
+    "x\nimport os\n_y",
+    "x[0]",
+    "",
+    "a" * 200,
+])
+def test_a_marquee_id_cannot_become_code(bad_id):
+    """data_key is spliced UNQUOTED into variable names
+    (_{{ data_key }}_text = ...), so a non-identifier id landed in the source
+    as code. ast.parse caught it, but the caller then got an opaque
+    "Generated code has a syntax error" rather than being told the id is bad."""
+    el = {"type": "marquee", "id": bad_id, "x": 0, "y": 0, "text": "hi"}
+    src = _generated(_payload(elements=[el]))       # must not raise
+    assert "__import__(" not in src
+    assert "os.system(" not in src
+    assert not _module_level_code(src)

test/test_composer_empty_block.py

@@ -0,0 +1,80 @@
+"""An element the template cannot draw must not produce an empty `if` block.
+
+manager.py.j2 wraps each element in `if width >= N:` (breakpoint) and/or
+`if int(time.time() * 2) % 2:` (blink), and the body comes from the per-type
+branches. A type with no branch contributed nothing, so the wrapper opened a
+block with no statements in it. ast.parse in _generate_plugin_files then
+failed and the caller was told only:
+
+    Generated code has a syntax error: expected an indented block after
+    'if' statement on line 49
+
+which names a line of generated source the user never sees. Confirmed against
+the code before the fix with a `group` element carrying minWidth.
+
+Two defences, both covered here: _preprocess_elements drops types the template
+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 re
+import sys
+from pathlib import Path
+
+import pytest
+
+sys.path.insert(0, str(Path(__file__).resolve().parent.parent))
+
+from web_interface.blueprints import composer as C  # noqa: E402
+
+TEMPLATE = (Path(__file__).resolve().parent.parent
+            / "web_interface/templates/v3/composer/manager.py.j2")
+
+BASE_META = {"id": "test-plugin", "name": "Clock", "author": "a",
+             "version": "1.0.0", "description": "d"}
+
+
+def generate(element):
+    return C._generate_plugin_files({
+        "metadata": BASE_META,
+        "elements": [element],
+        "dataModel": {"configVars": []},
+    })
+
+
+@pytest.mark.parametrize("wrapper", [
+    {"minWidth": 64},          # breakpoint block
+    {"blink": True},           # blink block
+    {"minWidth": 64, "blink": True},   # both, nested
+])
+@pytest.mark.parametrize("etype", ["group", "widget_9000", "section"])
+def test_undrawable_element_does_not_break_generation(etype, wrapper):
+    element = {"type": etype, "x": 0, "y": 0, "color": "#ffffff", **wrapper}
+    files = generate(element)          # must not raise ComposerInputError
+    assert "manager.py" in files
+
+
+def test_drawable_element_still_renders_inside_a_breakpoint():
+    files = generate({"type": "text", "text": "hi", "x": 0, "y": 0,
+                      "minWidth": 64, "color": "#ffffff"})
+    src = files["manager.py"]
+    assert "if width >= 64:" in src
+    assert "draw_text" in src
+
+
+def test_renderable_types_match_the_template_branches():
+    """The constant and the template must agree.
+
+    A type listed in the constant with no branch emits an empty block (the bug
+    above); a type with a branch but missing from the constant is silently
+    dropped from every generated plugin. Neither is visible without this check.
+    """
+    branches = set(re.findall(r"el\.type == '([a-z_]+)'", TEMPLATE.read_text()))
+    assert branches == set(C._RENDERABLE_ELEMENT_TYPES)
+
+
+def test_template_closes_the_branch_chain_with_a_fallback():
+    """Belt and braces: even if the constant drifts, no empty block escapes."""
+    text = TEMPLATE.read_text()
+    assert "{% else %}" in text
+    assert "pass  # element type" in text

test/test_composer_js_contracts.py

@@ -0,0 +1,147 @@
+"""Structural checks on the composer's JavaScript.
+
+There is no JS test runner in this repo, but three defects here are structural
+enough to assert on the parse tree, and each was a real bug:
+
+- Stroke widths inside _drawElement were left in canvas pixels while the
+  geometry around them scaled by SCALE, so at SCALE>1 every outline rendered
+  thinner than one LED pixel and the preview stopped matching the panel.
+- The `line` branch drew raw el.x0/el.y0/el.x1/el.y1, ignoring the anchor that
+  every other element type honours, so setting xAnchor moved everything except
+  lines -- and getBoundingBox had the same omission, leaving the hit box behind.
+- Four methods mutated exactly what _snapshot() serialises (metadata,
+  currentPreset) without calling it. _snapshot is the only caller of
+  _debouncedAutosave, so those changes were lost on reload and could not be
+  undone.
+"""
+import re
+from pathlib import Path
+
+import pytest
+
+tree_sitter = pytest.importorskip("tree_sitter")
+tree_sitter_javascript = pytest.importorskip("tree_sitter_javascript")
+
+JS_DIR = Path(__file__).resolve().parent.parent / "web_interface/static/v3/js/composer"
+CANVAS = JS_DIR / "composer-canvas.js"
+APP = JS_DIR / "composer-app.js"
+
+
+def _function_source(path: Path, name: str) -> str:
+    """Return the source of a top-level function declaration by name."""
+    src = path.read_bytes()
+    lang = tree_sitter.Language(tree_sitter_javascript.language())
+    tree = tree_sitter.Parser(lang).parse(src)
+    found = []
+
+    def walk(node):
+        if node.type == "function_declaration":
+            ident = node.child_by_field_name("name")
+            if ident is not None and src[ident.start_byte:ident.end_byte].decode() == name:
+                found.append(src[node.start_byte:node.end_byte].decode())
+        for c in node.children:
+            walk(c)
+
+    walk(tree.root_node)
+    assert found, f"{name} not found in {path.name}"
+    return found[0]
+
+
+def _method_source(path: Path, name: str) -> str:
+    """Return the source of a top-level object method by name."""
+    src = path.read_bytes()
+    lang = tree_sitter.Language(tree_sitter_javascript.language())
+    tree = tree_sitter.Parser(lang).parse(src)
+    found = []
+
+    def walk(node):
+        if node.type == "method_definition":
+            ident = node.child_by_field_name("name")
+            if ident is not None and src[ident.start_byte:ident.end_byte].decode() == name:
+                found.append(src[node.start_byte:node.end_byte].decode())
+        for c in node.children:
+            walk(c)
+
+    walk(tree.root_node)
+    assert found, f"{name} not found in {path.name}"
+    return found[0]
+
+
+def test_both_files_parse():
+    lang = tree_sitter.Language(tree_sitter_javascript.language())
+    parser = tree_sitter.Parser(lang)
+    for path in (CANVAS, APP):
+        tree = parser.parse(path.read_bytes())
+        errors = []
+
+        def walk(node):
+            if node.type == "ERROR" or node.is_missing:
+                errors.append(node.start_point[0] + 1)
+            for c in node.children:
+                walk(c)
+
+        walk(tree.root_node)
+        assert not errors, f"{path.name} has parse errors at lines {errors}"
+
+
+def test_element_strokes_scale_with_scale():
+    """No bare `ctx.lineWidth = 1` inside _drawElement.
+
+    Selection handles and the grid are drawn in canvas pixels deliberately and
+    live in other functions, so this is scoped to the element drawing routine.
+    """
+    body = _function_source(CANVAS, "_drawElement")
+    offenders = re.findall(r"ctx\.lineWidth\s*=\s*1\s*;", body)
+    assert not offenders, f"{len(offenders)} unscaled stroke width(s) in _drawElement"
+
+
+def test_line_branch_applies_the_anchor_offset():
+    """Scoped to _drawElement.
+
+    getBoundingBox has its own `case 'line': {` and appears first in the file,
+    so searching the whole text found *that* branch -- this assertion passed
+    with the draw branch's anchor offset removed. Verified: stripping it and
+    re-running gave 11/11 green.
+    """
+    body = _function_source(CANVAS, "_drawElement")
+    line_branch = body[body.index("case 'line': {"):]
+    line_branch = line_branch[:line_branch.index("case 'divider'")]
+    assert "ax - el.x0" in line_branch and "ay - el.y0" in line_branch, \
+        "line drawing ignores xAnchor/yAnchor"
+    assert "moveTo(el.x0 * s" not in line_branch, \
+        "line still drawn from unanchored endpoints"
+
+
+def test_line_bounding_box_applies_the_anchor_offset():
+    """The companion to the above: scoped to getBoundingBox specifically, so
+    the two tests cannot both be satisfied by the same branch."""
+    body = _function_source(CANVAS, "getBoundingBox")
+    box = body[body.index("case 'line'"):]
+    box = box[:box.index("case 'divider'")]
+    assert "ax - el.x0" in box, "line bounding box ignores the anchor"
+
+
+@pytest.mark.parametrize("method", [
+    "onBgColorChange",     # mutates metadata.bgColor
+    "setCustomSize",       # mutates currentPreset / MATRIX_W / MATRIX_H
+    "changePreset",        # mutates currentPreset / MATRIX_W / MATRIX_H
+    "applyPresetLabel",    # same, for sizes not in DISPLAY_PRESETS
+    "onColorChange",       # the one that was already fixed — keeps it fixed
+])
+def test_state_mutations_take_a_snapshot(method):
+    body = _method_source(APP, method)
+    assert "_snapshot()" in body, \
+        f"{method} changes snapshotted state without calling _snapshot()"
+    assert "isDirty = true" in body, f"{method} does not mark the design dirty"
+
+
+@pytest.mark.parametrize("method", ["changePreset", "applyPresetLabel"])
+def test_restore_path_stays_snapshot_free(method):
+    """_applyState and loadTemplate call these with {silent: true} while
+    restoring; snapshotting there would push restore steps onto the undo stack
+    and re-autosave the state just loaded."""
+    body = _method_source(APP, method)
+    assert "opts.silent" in body, f"{method} lost its silent guard"
+    snap = body.index("_snapshot()")
+    guard = body.index("!opts.silent")
+    assert guard < snap, f"{method} snapshots outside the !opts.silent guard"

web_interface/blueprints/composer.py

@@ -11,6 +11,7 @@ Routes:
 import ast
 import io
 import json
+import math
 import keyword
 import logging
 import os
@@ -150,9 +151,16 @@ def _safe_int(value, default: int = 0, lo: int | None = None,
     produced `x=0 or __import__("os").system("id")` in the generated source,
     which is valid Python and so passed the ast.parse check.
     """
+    # json.loads accepts Infinity/-Infinity/NaN by default and Flask's
+    # get_json passes them straight through, so a payload can hand this a
+    # non-finite float. int(inf) raises OverflowError, which is neither
+    # ValueError nor ComposerInputError -- it escaped both handlers and became
+    # a 500 with a traceback instead of a 422.
+    if isinstance(value, float) and not math.isfinite(value):
+        return default
     try:
         out = int(value)
-    except (TypeError, ValueError):
+    except (TypeError, ValueError, OverflowError):
         return default
     if lo is not None:
         out = max(lo, out)
@@ -161,11 +169,22 @@ def _safe_int(value, default: int = 0, lo: int | None = None,
     return out
 
 
+def _rgb_tuple(el: dict, keys, defaults) -> str:
+    """A colour tuple literal from coerced, clamped values under *keys*.
+
+    Several element types carry prefixed channels (emptyR/G/B, labelR/G/B)
+    rather than r/g/b. Those were interpolated straight into the generated
+    source, so they were an injection route exactly like an uncoerced
+    dimension. Every channel now goes through _safe_int.
+    """
+    return "(" + ", ".join(
+        str(_safe_int(el.get(k), d, 0, 255)) for k, d in zip(keys, defaults)
+    ) + ")"
+
+
 def _rgb_expr(el: dict, dr: int = 255, dg: int = 255, db: int = 255) -> str:
     """A colour tuple literal built from coerced, clamped channel values."""
-    return (f"({_safe_int(el.get('r'), dr, 0, 255)}, "
+    return _rgb_tuple(el, ('r', 'g', 'b'), (dr, dg, db))
-            f"{_safe_int(el.get('g'), dg, 0, 255)}, "
-            f"{_safe_int(el.get('b'), db, 0, 255)})")
 
 
 def _compute_pos_expr(val, anchor: str | None, dim_var: str) -> str:
@@ -213,6 +232,16 @@ def _aligned_x_expr(x_base_expr: str, text_align: str, char_count: int, char_w:
     return x_base_expr
 
 
+#: Element types manager.py.j2 has a drawing branch for. Kept next to the
+#: preprocessor because the two must agree: a type here with no branch emits an
+#: empty block, and a type with a branch but missing here is silently dropped.
+_RENDERABLE_ELEMENT_TYPES = frozenset({
+    'text', 'dynamic_text', 'clock', 'countdown', 'rectangle', 'arc',
+    'ellipse', 'pixel', 'rounded_rectangle', 'pips', 'sparkline', 'gauge',
+    'marquee', 'progress_bar',
+})
+
+
 def _preprocess_elements(elements: list) -> list:
     """Expand raw element dicts into template-ready dicts with anchor expressions.
 
@@ -231,6 +260,16 @@ def _preprocess_elements(elements: list) -> list:
         if t == 'section':
             continue
 
+        # A type the template has no branch for still gets its breakpoint and
+        # blink wrappers emitted, and those would open an `if` with nothing in
+        # it -- ast.parse then fails and the caller is told only "Generated
+        # code has a syntax error". Drop it here instead. The template also
+        # emits a `pass` fallback, so a type added to the canvas before its
+        # branch exists degrades to a no-op rather than a broken plugin.
+        if t not in _RENDERABLE_ELEMENT_TYPES:
+            logger.info("composer: skipping element type %r with no template branch", t)
+            continue
+
         x_anchor = el.get('xAnchor') or None
         y_anchor = el.get('yAnchor') or None
         p['min_width'] = int(el.get('minWidth', 0) or 0)
@@ -290,8 +329,8 @@ def _preprocess_elements(elements: list) -> list:
         elif t == 'rectangle':
             x_expr = _compute_pos_expr(el.get('x', 0), x_anchor, 'width')
             y_expr = _compute_pos_expr(el.get('y', 0), y_anchor, 'height')
-            w = el.get('width', 10)
+            w = _safe_int(el.get('width'), 10, 0, 4096)
-            h = el.get('height', 8)
+            h = _safe_int(el.get('height'), 8, 0, 4096)
             p['x_expr'] = x_expr
             p['y_expr'] = y_expr
             # x2/y2 as runtime expressions to support anchored positions
@@ -336,7 +375,7 @@ def _preprocess_elements(elements: list) -> list:
             p['bar_height'] = int(el.get('barHeight', 6))
             binding = el.get('binding', {})
             p['binding_key'] = binding.get('key', '')
-            p['fill_tuple'] = f"({el.get('r', 100)}, {el.get('g', 200)}, {el.get('b', 100)})"
+            p['fill_tuple'] = _rgb_tuple(el, ('r', 'g', 'b'), (100, 200, 100))
             bg = (
                 [el.get('bgR', 30), el.get('bgG', 30), el.get('bgB', 30)]
                 if el.get('hasBg', True) else None
@@ -352,8 +391,8 @@ def _preprocess_elements(elements: list) -> list:
         elif t == 'arc':
             x_expr = _compute_pos_expr(el.get('x', 0), x_anchor, 'width')
             y_expr = _compute_pos_expr(el.get('y', 0), y_anchor, 'height')
-            w = el.get('width', 24)
+            w = _safe_int(el.get('width'), 24, 0, 4096)
-            h = el.get('height', 24)
+            h = _safe_int(el.get('height'), 24, 0, 4096)
             p['x_expr'] = x_expr
             p['y_expr'] = y_expr
             p['x2_expr'] = f"({x_expr}) + {w}"
@@ -367,8 +406,8 @@ def _preprocess_elements(elements: list) -> list:
         elif t == 'ellipse':
             x_expr = _compute_pos_expr(el.get('x', 0), x_anchor, 'width')
             y_expr = _compute_pos_expr(el.get('y', 0), y_anchor, 'height')
-            w = el.get('width', 24)
+            w = _safe_int(el.get('width'), 24, 0, 4096)
-            h = el.get('height', 12)
+            h = _safe_int(el.get('height'), 12, 0, 4096)
             p['x_expr'] = x_expr
             p['y_expr'] = y_expr
             p['x2_expr'] = f"({x_expr}) + {w}"
@@ -394,8 +433,8 @@ def _preprocess_elements(elements: list) -> list:
         elif t == 'rounded_rectangle':
             x_expr = _compute_pos_expr(el.get('x', 0), x_anchor, 'width')
             y_expr = _compute_pos_expr(el.get('y', 0), y_anchor, 'height')
-            w = el.get('width', 24)
+            w = _safe_int(el.get('width'), 24, 0, 4096)
-            h = el.get('height', 10)
+            h = _safe_int(el.get('height'), 10, 0, 4096)
             p['x_expr'] = x_expr
             p['y_expr'] = y_expr
             p['x2_expr'] = f"({x_expr}) + {w}"
@@ -434,8 +473,8 @@ def _preprocess_elements(elements: list) -> list:
             p['show_empty'] = bool(el.get('showEmpty', True))
             binding = el.get('binding', {})
             p['binding_key'] = binding.get('key', '')
-            p['fill_tuple'] = f"({el.get('r', 255)}, {el.get('g', 200)}, {el.get('b', 0)})"
+            p['fill_tuple'] = _rgb_tuple(el, ('r', 'g', 'b'), (255, 200, 0))
-            p['empty_tuple'] = f"({el.get('emptyR', 50)}, {el.get('emptyG', 50)}, {el.get('emptyB', 50)})"
+            p['empty_tuple'] = _rgb_tuple(el, ('emptyR', 'emptyG', 'emptyB'), (50, 50, 50))
             p['blink'] = bool(el.get('blink', False))
 
         elif t == 'sparkline':
@@ -449,7 +488,7 @@ def _preprocess_elements(elements: list) -> list:
             p['bar_spacing'] = max(0, int(el.get('barSpacing', 1)))
             binding = el.get('binding', {})
             p['binding_key'] = binding.get('key', '')
-            p['fill_tuple'] = f"({el.get('r', 80)}, {el.get('g', 200)}, {el.get('b', 120)})"
+            p['fill_tuple'] = _rgb_tuple(el, ('r', 'g', 'b'), (80, 200, 120))
             bg = [el.get('bgR', 30), el.get('bgG', 30), el.get('bgB', 30)] if el.get('hasBg', False) else None
             p['bg_tuple'] = _as_fill_filter(bg)
             p['blink'] = bool(el.get('blink', False))
@@ -457,8 +496,8 @@ def _preprocess_elements(elements: list) -> list:
         elif t == 'gauge':
             x_expr = _compute_pos_expr(el.get('x', 0), x_anchor, 'width')
             y_expr = _compute_pos_expr(el.get('y', 0), y_anchor, 'height')
-            w = el.get('width', 32)
+            w = _safe_int(el.get('width'), 32, 0, 4096)
-            h = el.get('height', 32)
+            h = _safe_int(el.get('height'), 32, 0, 4096)
             p['x_expr'] = x_expr
             p['y_expr'] = y_expr
             p['x2_expr'] = f"({x_expr}) + {w}"
@@ -477,7 +516,7 @@ def _preprocess_elements(elements: list) -> list:
             font_key = el.get('font', 'four_by_six')
             p['font_attr'] = _FONT_ATTR_MAP.get(font_key, 'extra_small_font')
             p['show_label'] = bool(el.get('showLabel', True))
-            p['label_tuple'] = f"({el.get('labelR', 200)}, {el.get('labelG', 200)}, {el.get('labelB', 200)})"
+            p['label_tuple'] = _rgb_tuple(el, ('labelR', 'labelG', 'labelB'), (200, 200, 200))
             p['blink'] = bool(el.get('blink', False))
 
         elif t == 'marquee':
@@ -490,9 +529,15 @@ def _preprocess_elements(elements: list) -> list:
             p['gap'] = int(el.get('gap', 16))
             p['scroll_speed'] = max(1, int(el.get('scrollSpeed', 1)))
             p['direction'] = el.get('direction', 'left')
-            # Data key stored in self._data for stateful scrolling across display() calls
+            # Data key stored in self._data for stateful scrolling across
-            raw_id = str(el.get('id', 0)).replace('-', '_')
+            # display() calls. It is spliced UNQUOTED into variable names
-            p['data_key'] = f"mq_{raw_id}"
+            # (_{{ data_key }}_text = ...), so anything that is not an
+            # identifier character lands in the generated source as code.
+            # ast.parse catches the result, but the caller then gets an opaque
+            # "Generated code has a syntax error" instead of being told the id
+            # is unusable. Restrict it to identifier characters and bound it.
+            raw_id = re.sub(r'[^A-Za-z0-9_]', '_', str(el.get('id', 0)))[:64]
+            p['data_key'] = f"mq_{raw_id or '0'}"
             p['blink'] = bool(el.get('blink', False))
 
         result.append(p)

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

@@ -480,7 +480,11 @@ function composerApp() {
       this.currentPreset = `${w}×${h}`;
       this.SCALE = w <= 64 ? 6 : w <= 128 ? 4 : 2;
       this._applyScale();
-      if (!opts.silent) this.render();
+      if (!opts.silent) {
+        this.isDirty = true;
+        this._snapshot();
+        this.render();
+      }
     },
 
     changePreset(presetLabel, opts = {}) {
@@ -496,7 +500,11 @@ function composerApp() {
         canvas.style.width  = (this.MATRIX_W * this.SCALE) + 'px';
         canvas.style.height = (this.MATRIX_H * this.SCALE) + 'px';
       }
-      if (!opts.silent) this.render();
+      if (!opts.silent) {
+        this.isDirty = true;
+        this._snapshot();
+        this.render();
+      }
     },
 
     // ── Rendering ─────────────────────────────────────────────────────
@@ -626,6 +634,10 @@ function composerApp() {
       this.currentPreset = `${w}×${h}`;
       this.SCALE = w <= 64 ? 6 : w <= 128 ? 4 : 2;
       this._applyScale();
+      // currentPreset is part of the snapshot; a custom size set here was
+      // otherwise dropped on reload and could not be undone.
+      this.isDirty = true;
+      this._snapshot();
       this._setStatus(`Canvas set to ${w}×${h}`, 'info');
     },
     zoomFit() {
@@ -645,6 +657,11 @@ function composerApp() {
         g: parseInt(hex.slice(3, 5), 16),
         b: parseInt(hex.slice(5, 7), 16),
       };
+      // _snapshot serialises metadata and is the only caller of
+      // _debouncedAutosave, so without this the background colour was lost on
+      // reload and could not be undone. Same defect as onColorChange.
+      this.isDirty = true;
+      this._snapshot();
       this.render();
     },
 

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

@@ -208,12 +208,16 @@ window.ComposerCanvas = (() => {
         return { x: ax, y: ay, w: el.width, h: el.height };
       case 'pixel':
         return { x: ax, y: ay, w: 1, h: 1 };
-      case 'line':
+      case 'line': {
+        // Same anchor offset the draw path applies, or the hit box sits where
+        // the line used to be.
+        const dx = ax - el.x0, dy = ay - el.y0;
         return {
-          x: Math.min(el.x0, el.x1), y: Math.min(el.y0, el.y1),
+          x: Math.min(el.x0, el.x1) + dx, y: Math.min(el.y0, el.y1) + dy,
           w: Math.max(1, Math.abs(el.x1 - el.x0)),
           h: Math.max(1, Math.abs(el.y1 - el.y0)),
         };
+      }
       case 'divider':
         return el.orientation === 'horizontal'
           ? { x: 0,  y: ay, w: matrixW, h: 1 }
@@ -384,7 +388,9 @@ window.ComposerCanvas = (() => {
           }
           if (el.hasOutline) {
             ctx.strokeStyle = `rgb(${el.outR},${el.outG},${el.outB})`;
-            ctx.lineWidth = 1;
+            // 1 LED pixel, not 1 canvas pixel: at SCALE>1 an unscaled stroke
+            // renders thinner than the geometry it outlines.
+            ctx.lineWidth = s;
             ctx.strokeRect(rx, ry, rw, rh);
           }
           break;
@@ -393,8 +399,11 @@ window.ComposerCanvas = (() => {
         case 'ellipse': {
           const cx = (ax + el.width / 2) * s;
           const cy = (ay + el.height / 2) * s;
-          const rx = (el.width / 2) * s;
+          const lwPx = s;  // 1 LED pixel
-          const ry = (el.height / 2) * s;
+          // Inset by half the stroke, which straddles the path, so the outline
+          // stays within the element's bounds.
+          const rx = Math.max(0, (el.width / 2) * s - (el.hasOutline ? lwPx / 2 : 0));
+          const ry = Math.max(0, (el.height / 2) * s - (el.hasOutline ? lwPx / 2 : 0));
           ctx.beginPath();
           ctx.ellipse(cx, cy, rx, ry, 0, 0, Math.PI * 2);
           if (el.hasFill) {
@@ -403,7 +412,7 @@ window.ComposerCanvas = (() => {
           }
           if (el.hasOutline) {
             ctx.strokeStyle = `rgb(${el.outR},${el.outG},${el.outB})`;
-            ctx.lineWidth = 1;
+            ctx.lineWidth = lwPx;
             ctx.stroke();
           }
           break;
@@ -412,15 +421,16 @@ window.ComposerCanvas = (() => {
         case 'arc': {
           const cx = (ax + el.width / 2) * s;
           const cy = (ay + el.height / 2) * s;
-          const rx = (el.width / 2) * s;
+          const lwPx = Math.max(1, el.lineWidth || 2) * s;
-          const ry = (el.height / 2) * s;
+          const rx = Math.max(0, (el.width / 2) * s - lwPx / 2);
+          const ry = Math.max(0, (el.height / 2) * s - lwPx / 2);
           // PIL: 0°=right, clockwise. Canvas: same with anticlockwise=false
           const startRad = (el.startAngle ?? 0) * Math.PI / 180;
           const endRad = (el.endAngle ?? 270) * Math.PI / 180;
           ctx.beginPath();
           ctx.ellipse(cx, cy, rx, ry, 0, startRad, endRad, false);
           ctx.strokeStyle = `rgb(${el.r},${el.g},${el.b})`;
-          ctx.lineWidth = Math.max(1, el.lineWidth || 2);
+          ctx.lineWidth = lwPx;
           ctx.stroke();
           break;
         }
@@ -443,18 +453,21 @@ window.ComposerCanvas = (() => {
           }
           if (el.hasOutline) {
             ctx.strokeStyle = `rgb(${el.outR},${el.outG},${el.outB})`;
-            ctx.lineWidth = 1;
+            ctx.lineWidth = s;
             ctx.stroke();
           }
           break;
         }
 
         case 'line': {
+          // ax/ay resolve from el.x0 for a line, so this is the anchor offset.
+          // Without it a line stayed put while every other type moved.
+          const dx = ax - el.x0, dy = ay - el.y0;
           ctx.strokeStyle = `rgb(${el.r},${el.g},${el.b})`;
-          ctx.lineWidth = Math.max(1, el.lineWidth || 1);
+          ctx.lineWidth = Math.max(1, el.lineWidth || 1) * s;
           ctx.beginPath();
-          ctx.moveTo(el.x0 * s, el.y0 * s);
+          ctx.moveTo((el.x0 + dx) * s, (el.y0 + dy) * s);
-          ctx.lineTo(el.x1 * s, el.y1 * s);
+          ctx.lineTo((el.x1 + dx) * s, (el.y1 + dy) * s);
           ctx.stroke();
           break;
         }
@@ -462,7 +475,7 @@ window.ComposerCanvas = (() => {
         case 'divider': {
           const isH = (el.orientation || 'horizontal') === 'horizontal';
           ctx.strokeStyle = `rgb(${el.r},${el.g},${el.b})`;
-          ctx.lineWidth = 1;
+          ctx.lineWidth = s;
           ctx.beginPath();
           if (isH) {
             ctx.moveTo(0, ay * s + 0.5);
@@ -614,7 +627,7 @@ window.ComposerCanvas = (() => {
           }
           if (el.hasOutline) {
             ctx.strokeStyle = `rgb(${el.outR ?? 100},${el.outG ?? 100},${el.outB ?? 100})`;
-            ctx.lineWidth = 1;
+            ctx.lineWidth = s;
             ctx.strokeRect(rx, ry, bw * s, bh * s);
           }
           break;

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

@@ -288,6 +288,8 @@ class {{ class_name }}(BasePlugin):
 {{ pi }}        [_pb_x, _pb_y, _pb_x + _pb_fill_w, _pb_y + {{ el.bar_height }}],
 {{ pi }}        fill={{ el.fill_tuple }},
 {{ pi }}    )
+{% else %}
+{{ pi }}pass  # element type "{{ el.type }}" draws nothing
 {% endif %}
 {% endfor %}
             # ── End elements ───────────────────────────────────────────────