rick/LEDMatrix
1
0
Fork 0
mirror of https://github.com/ChuckBuilds/LEDMatrix.git synced 2026-08-22 19:08:17 +00:00
Code Issues Packages Projects Releases Wiki Activity
1,925 Commits 64 Branches 11 Tags
37fc1b56b5ee4f931a38562ea4c4af1b51d60697
Commit Graph
5 Commits
Author SHA1 Message Date
ChuckBuildsandClaude Opus 5 37fc1b56b5 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
 2026-08-22 14:35:30 -04:00
ChuckBuildsandClaude Opus 5 1a0864e5d4 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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
 2026-08-22 13:11:47 -04:00
ChuckBuildsandClaude Opus 5 d42593e7ce Merge main into feat/plugin-composer, and fix three review findings 
The branch was 57 commits behind and conflicting. I had put the rebase
aside earlier as needing the author's eyes, on the grounds that the PR is
+5091 lines -- but that was the wrong measure. The actual conflict was a
single hunk in app.css, where this branch adds .md\:inline and main added
.md\:block and .md\:w-auto at the same place. All three are kept.

Merging rather than rebasing: the branch is public and 57 commits behind,
so a rebase would rewrite shared history for a force-push.

Three findings fixed on top:

A missing `text` or `format` was a 500. `p` is a copy of the raw element
and the defaults were applied to the locals t1/fmt1 only, so an element
omitting either key left it absent, manager.py.j2 rendered
`{{ el.text | tojson }}` over a jinja2.Undefined, and tojson raised
TypeError -- which no handler catches:

    text without 'text':    TypeError: Object of type Undefined is not
                            JSON serializable
    clock without 'format': same

Both keys are now set explicitly. Verified: removing either assignment
fails 4 of the new tests.

E741 on my own injection-test file: two `for i, l in enumerate(...)`
loops, which ruff rejects and would fail a lint-gated build. Renamed.
Ruff now clean on all three files this PR touches.

Not done: registering composer_bp. This PR's own description gates it --
"Not yet wired up ... tracking as a follow-up", with an unchecked box for
"Register composer_bp in app.py before merging or exposing this route" --
so it is a deliberate decision, not an oversight. Confirmed the blueprint
appears in no register_blueprint call outside this branch's tests, which
also means the code-injection fixed earlier in this PR was never
reachable in a deployed instance. Worth fixing before the route is
exposed; not worth exposing the route to satisfy a review comment.

Verified on the merged tree: 3850 passed, 1 failed, 60 skipped. The
failure is test_install_lowmem's tmpfs assumption, which is fixed in #492
and not yet on main. The static audit now passes 3/3 -- the twelve
classes it flagged before were defined on main all along and only looked
missing because this branch was behind.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
 2026-08-21 20:51:14 -04:00
ChuckBuildsandClaude Opus 5 986f74e38b fix(composer): reject config keys that shadow plugin state 
Five review findings, plus the two bandit reported.

Config variable keys were checked against an identifier regex only.
Python keywords slipped past it and were caught downstream by ast.parse,
but reported as

    Generated code has a syntax error: invalid syntax (<unknown>, line 17)

which names neither the field nor the value. They are now refused by
name, soft keywords ('match', 'case') included.

Worse, a key matching a BasePlugin attribute generated *valid* code that
silently clobbered plugin state. 'config' is the sharp one: the
assignment lands immediately after super().__init__(), so

    self.config = config.get("config", "x")

replaces the plugin's config dict with a string, and every later
self.config.get(...) fails at runtime. Refused now, along with logger,
display_manager, cache_manager, plugin_id, enabled, self and the
lifecycle method names. A test pins the ordering assumption that reserved
list rests on, so it fails if config vars are ever emitted before
super().__init__() instead.

Also:

- The silent `except Exception: pass` around manifest parsing now logs.
  It left "partial import produced nothing" indistinguishable from a
  malformed manifest. (bandit B110)
- list_plugins() called iterdir() on a directory that may not exist --
  a fresh install or a bad path returned 500 instead of an empty list.
- metadata.id is stripped in the two route handlers, matching
  _generate_plugin_files, which strips before validating. Without it
  " my-plugin " generated fine and then failed the id check at install,
  reading as a generator bug.
- The jinja Environment's autoescape=False now says why: these templates
  emit Python, and escaping a quote to &#34; inside generated code would
  break it. Safety comes from the values instead -- _safe_int, _rgb_expr
  and _reject_source_breaking, all covered by the injection suite.
  (bandit B701, marked nosec with that rationale)

bandit on composer.py: 2 findings -> 0.

Verified: 156 tests across the two composer suites. Removing either new
key check fails 9.

Not reproduced: the suggestion to emit `pass` so a conditional block is
never empty. 'line' and 'divider' render through a different template
branch and 'section' emits nothing at all, so no element type available
here produces an `if width >= N:` with an empty body. Left alone rather
than changing template output speculatively.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
 2026-08-21 19:38:54 -04:00
ChuckBuildsandClaude Opus 5 e450a6dfb6 fix(composer): stop payload text reaching generated Python as code 
Review flagged this as critical and it is: the composer builds manager.py
by interpolating payload values into source text, /api/install writes
that file into plugins_dir, and the plugin loader imports and executes
it. The ast.parse check further down rejects only *invalid* syntax, and
an injected `import os` is perfectly valid.

Confirmed against the code before this commit. A plugin name carrying a
triple quote closes the module docstring and everything after it becomes
module-level code:

    generated manager.py parses: True
    injected module-level statements: ['import os', 'PWNED = os.getuid()']

and a geometry value is interpolated verbatim, because the parameter is
annotated int but arrives as JSON:

    _compute_pos_expr('0 or __import__("os").system("id")', 'right', 'width')
      -> 'width - 0 or __import__("os").system("id")'
    generated source: x=0 or __import__("os").system("id"),

Three fixes. _safe_int coerces and optionally clamps, and
_compute_pos_expr applies it to its own argument -- which covers all
twenty-odd call sites at once rather than patching each. _rgb_expr does
the same for the eight colour interpolations, clamping channels to
0-255. Line endpoints and widths go through it too.

For the docstring, _reject_source_breaking refuses a plugin name
containing a quote, backslash or newline. Rejecting rather than escaping:
these are display names, none of that belongs in one, and a clear "Plugin
name cannot contain a double quote." beats silently mangling what the
user typed.

Verified: all three exploits now refused or neutered, and each defence
mutation-checked separately --

    coercion removed in _compute_pos_expr ->  8 failed
    docstring guard removed               ->  5 failed
    colour channels interpolated raw      -> 13 failed

87 tests, covering seven expression payloads across seven geometry
fields and three colour channels, five literal-breaking names, and the
clean case asserting a normal payload still yields no module-level
statements at all.

One aside: the first version of this test file put the exploit string
in its own module docstring, which closed it and made the file a syntax
error -- the same bug, one level up. It now describes the payload rather
than embedding it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
 2026-08-21 19:03:13 -04:00