From 37fc1b56b5ee4f931a38562ea4c4af1b51d60697 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sat, 22 Aug 2026 14:35:30 -0400 Subject: [PATCH] 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) Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW --- test/test_composer_code_injection.py | 68 ++++++++++++++++++++++++++++ test/test_composer_js_contracts.py | 22 ++++++--- web_interface/blueprints/composer.py | 49 +++++++++++++++----- 3 files changed, 121 insertions(+), 18 deletions(-) diff --git a/test/test_composer_code_injection.py b/test/test_composer_code_injection.py index fd022aed..a9527098 100644 --- a/test/test_composer_code_injection.py +++ b/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 @@ -231,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) diff --git a/test/test_composer_js_contracts.py b/test/test_composer_js_contracts.py index 7e612ee6..202e11b1 100644 --- a/test/test_composer_js_contracts.py +++ b/test/test_composer_js_contracts.py @@ -96,18 +96,28 @@ def test_element_strokes_scale_with_scale(): def test_line_branch_applies_the_anchor_offset(): - text = CANVAS.read_text() - line_branch = text[text.index("case 'line': {"):] + """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 "el.x0 * s" not in line_branch, "line still drawn from unanchored endpoints" + assert "moveTo(el.x0 * s" not in line_branch, \ + "line still drawn from unanchored endpoints" def test_line_bounding_box_applies_the_anchor_offset(): - text = CANVAS.read_text() - i = text.index("case 'line':", text.index("getBoundingBox")) - box = text[i:i + 400] + """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" diff --git a/web_interface/blueprints/composer.py b/web_interface/blueprints/composer.py index e122b339..843e620a 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 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)}, " - f"{_safe_int(el.get('g'), dg, 0, 255)}, " - f"{_safe_int(el.get('b'), db, 0, 255)})") + return _rgb_tuple(el, ('r', 'g', 'b'), (dr, dg, db)) def _compute_pos_expr(val, anchor: str | None, dim_var: str) -> str: @@ -356,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 @@ -454,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['empty_tuple'] = f"({el.get('emptyR', 50)}, {el.get('emptyG', 50)}, {el.get('emptyB', 50)})" + p['fill_tuple'] = _rgb_tuple(el, ('r', 'g', 'b'), (255, 200, 0)) + p['empty_tuple'] = _rgb_tuple(el, ('emptyR', 'emptyG', 'emptyB'), (50, 50, 50)) p['blink'] = bool(el.get('blink', False)) elif t == 'sparkline': @@ -469,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)) @@ -497,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': @@ -510,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 - raw_id = str(el.get('id', 0)).replace('-', '_') - p['data_key'] = f"mq_{raw_id}" + # Data key stored in self._data for stateful scrolling across + # display() calls. It is spliced UNQUOTED into variable names + # (_{{ 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)