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>
CodeRabbit flagged several payload-derived int() conversions in composer.py
that could raise ValueError instead of clamping like every other coerced
value in the module (_as_rgb_filter and its callers -- fillR/G/B, outR/G/B,
bgR/G/B, trackR/G/B -- plus min_width, lineSpacing, barWidth/Height,
start/endAngle, borderRadius, pip*, sparkline bar*, marquee gap/scrollSpeed).
Route them all through _safe_int for consistency with the rest of the module
and to avoid the generic 422 a raw ValueError produces.
Also:
- FileReader.onerror was unset in importDesign(), so a failed file read
produced no status message.
- URL.revokeObjectURL() ran synchronously right after link.click() in both
exportDesign() and generateZip(); deferred via setTimeout(..., 0) so the
download reliably starts before the object URL is revoked.
- _module_level_code() (test helper) filtered ast.ImportFrom but not
ast.Import, so a bare `import os` payload wouldn't be caught by the
helper itself, even though downstream assertions still caught it.
Added regression tests for the fillR/G/B injection + clamping path, which
had no coverage (existing tests only covered the r/g/b _rgb_expr path).
Skipped as not worth the churn (CodeRabbit nitpicks, both "Trivial/Low value"):
- test_composer_empty_block.py's branch regex not matching digit-containing
type names -- no such type exists today.
- test_composer_path_containment.py's `C.composer_bp.name and Flask(...)`
truthiness guard -- cosmetic, blueprint name is never falsy.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XKc832xpVEx3C3W5BVqQ5Z
Two outstanding CodeRabbit outside-diff findings on this PR:
- The divider branch applied its 0.5px centering offset after scaling
(`ay * s + 0.5`) instead of before it (`(ay + 0.5) * s`), so at SCALE>1
the stroke bled into the preceding LED row/column instead of straddling
its own.
- A small imported gauge with a wide lineWidth (e.g. width=1, height=1,
lineWidth=3) produced a negative radius, which ctx.ellipse() throws
IndexSizeError on, aborting render() for every element still to be
drawn. Radii are now clamped to zero.
Verified both against current code (neither was fixed by later commits
on this branch) and mutation-checked: reverting either fix fails the new
test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
composer_bp defined the whole Plugin Composer feature -- the /composer/
page and all its API routes -- but web_interface/app.py never imported or
registered it, so every composer URL 404'd in the running app (every
composer test builds its own minimal Flask app and registers the
blueprint directly, which is why this went unnoticed). Wire it up the
same way pages_v3/api_v3 are: import, set config_manager/plugin_manager/
plugins_dir/project_root, register_blueprint(url_prefix='/composer') --
matching the prefix composer.html and composer-app.js already hardcode.
Also closes the other still-open half of a CodeRabbit finding: manager.py.j2
already guards element types the template has no branch for, but a
dynamic_text element with binding.source other than 'config' hit the same
empty-if-block bug one level deeper (its own inner if produced nothing).
Added the same pass fallback.
Verified both against current code before fixing; the other 8 findings
from that review were already fixed in earlier commits on this branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude