From 1a0864e5d4f2408af0d13671623b8d581bf85001 Mon Sep 17 00:00:00 2001 From: ChuckBuilds Date: Sat, 22 Aug 2026 13:11:47 -0400 Subject: [PATCH] fix(composer): clamp width/height before they reach generated source Code injection, found by chasing why a security test could not have caught it. _preprocess_elements built the far corner of five shapes by interpolating the payload's width/height straight into generated Python: w = el.get('width', 10) p['x2_expr'] = f"({x_expr}) + {w}" so a rectangle with width='0 or __import__("os").system("id")' generated [0, 0, (0) + 0 or __import__("os").system("id"), (0) + 8], inside a manager.py that /api/install writes to disk and the plugin loader imports and executes. rectangle, arc, ellipse, rounded_rectangle and gauge all share the pattern. Both fields now go through _safe_int, like every other geometry value. Unreachable today only because composer_bp is still unregistered -- the same caveat as the docstring injection fixed earlier in this PR. Why the existing test missed it ------------------------------- test_a_non_numeric_geometry_value_cannot_reach_the_source drove its payloads through a "line" element. manager.py.j2 has never had a `line` branch, so _preprocess_elements produced nothing for it and no value it set could reach the generated source. Every assertion passed trivially, against code that was in fact vulnerable. The test has been vacuous since it was written; the _RENDERABLE_ELEMENT_TYPES constant added in the previous commit only made the cause legible. It now runs across the five types that actually render, over x/y/width/height: 40 of those cases fail with the clamping reverted, where the old version passed 100%. A second test asserts every type used by the injection suite is in _RENDERABLE_ELEMENT_TYPES, so the suite cannot quietly go vacuous again. Also: _payload set "config_vars", but _generate_plugin_files reads data['dataModel']['configVars']. Nothing passed through that key was ever read. Fixed so config-var tests exercise the real path. Full suite: 3967 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 | 50 ++++++++++++++++++++++++---- web_interface/blueprints/composer.py | 20 +++++------ 2 files changed, 53 insertions(+), 17 deletions(-) diff --git a/test/test_composer_code_injection.py b/test/test_composer_code_injection.py index c09e8797..fd022aed 100644 --- a/test/test_composer_code_injection.py +++ b/test/test_composer_code_injection.py @@ -48,7 +48,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 +80,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"]) -def test_a_non_numeric_geometry_value_cannot_reach_the_source(evil, field): - el = {"type": "line", "id": "l1", "x0": 0, "y0": 0, "x1": 10, "y1": 10, - "anchor_x": "right", "anchor_y": "bottom"} +@pytest.mark.parametrize("field", ["x", "y", "width", "height"]) +def test_a_non_numeric_geometry_value_cannot_reach_the_source(etype, base, evil, field): + """width/height were interpolated raw into the generated source. + + 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) diff --git a/web_interface/blueprints/composer.py b/web_interface/blueprints/composer.py index 4c3f3d78..e122b339 100644 --- a/web_interface/blueprints/composer.py +++ b/web_interface/blueprints/composer.py @@ -310,8 +310,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) - h = el.get('height', 8) + w = _safe_int(el.get('width'), 10, 0, 4096) + 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 @@ -372,8 +372,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) - h = el.get('height', 24) + w = _safe_int(el.get('width'), 24, 0, 4096) + 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}" @@ -387,8 +387,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) - h = el.get('height', 12) + w = _safe_int(el.get('width'), 24, 0, 4096) + 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}" @@ -414,8 +414,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) - h = el.get('height', 10) + w = _safe_int(el.get('width'), 24, 0, 4096) + 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}" @@ -477,8 +477,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) - h = el.get('height', 32) + w = _safe_int(el.get('width'), 32, 0, 4096) + 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}"