mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-08-22 02:48:15 +00:00
d42593e7ce3ef7507676ccda7bbbe5b45ded7c02
10
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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
|
||
|
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 " 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
|
||
|
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
|
||
|
ChuckBuildsandClaude Opus 5
|
5133643600 |
fix(composer): build the font path from the allowlist entry
secure_filename cleared the plugin-directory alerts: CodeQL went from 19
to 5, and from 16 high-severity to 2. The two that remain are in
serve_font, which is gated by a frozenset of three exact filenames -- so
nothing was exploitable -- but the name reaching the filesystem was still
the request value.
It now comes from the matched allowlist entry. Identical strings, so
runtime behaviour is unchanged; the difference is that the filename is
provably a module constant rather than a guarded piece of user input.
The first test I wrote for this proved nothing. It asserted 404 on
traversal payloads, but Flask's router will not match a path segment
containing '/', and the rest 404 simply because no such file exists --
so removing the allowlist entirely still passed. Replaced with a readable
file planted next to the fonts:
fonts/id_rsa.ttf -> 404, body does not contain its contents
which fails with "a readable non-allowlisted file was served" the moment
the gate is removed.
45 tests.
Left alone: three medium py/stack-trace-exposure alerts on the
_generate_plugin_files handlers, which return str(exc) for
ComposerInputError. Its seven raise sites are all authored literals
("Author is required.", "Config variable key X is not a valid Python
identifier."), so no traceback or path is exposed. Clearing them means
either replacing that feedback with a generic string or restructuring
validation to return errors instead of raising -- a change to the
author's design, made blind, since CodeQL cannot be run locally to
confirm it would even work. That is a decision, not a cleanup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
|
||
|
ChuckBuildsandClaude Opus 5
|
5929190e36 |
fix(composer): sanitise the id with the form CodeQL recognises
Previous attempt got the count from 22 down to 19 but left the 16
path-injection alerts untouched: CodeQL carries taint through
_plugin_dir's return value and does not treat an internal realpath /
commonpath guard as a sanitiser.
secure_filename is one it does model. It is also a no-op on every id the
regex accepts -- verified across the accepted alphabet, 4000 generated
ids, zero altered -- so it cannot rewrite a caller's id into a different
plugin's directory. The equality check makes that explicit: if it changes
anything, the id was not one we accept, and we refuse rather than
silently redirect.
Found a real bug while testing the layers separately: '.' resolved to the
plugins root, and install() calls shutil.rmtree(target) when force is
set, so an id of '.' would have deleted every installed plugin. The regex
blocks it today, but the containment layer was allowing candidate == base
on the grounds that the base is not "outside" itself. A plugin directory
must be a child, never the root.
That came out of writing the isolated tests. Removing containment did not
fail anything, because secure_filename rejects traversal first -- which
made a redundant layer look load-bearing. Each layer is now neutralised
in turn so the one under test is the only thing standing:
containment removed -> FAIL (13 payloads reach the base or past it)
candidate == base allowed -> FAIL ('.' resolves to the plugins root)
commonpath -> startswith -> FAIL (sibling "plugins-evil" accepted)
secure_filename bypassed -> pass, containment covers it
The last is honest rather than a gap: with containment in place the
sanitiser has nothing left to block, and its value here is CodeQL
recognition plus a second barrier if containment is ever weakened.
Also corrected an assertion in the previous commit's test, which counted
any non-None result as an escape. '....', '~' and 'a\..\..' are ordinary
directory names on Linux and resolve safely inside the base; treating
them as escapes made the test fail on correct code.
35 tests. The 5 test_web_api.py failures are pre-existing on this branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
|
||
|
ChuckBuildsandClaude Opus 5
|
79ba93f5a6 |
fix(composer): recognisable path containment, and define md:inline
Follow-up to the previous commit, which made the CodeQL count worse
rather than better: 19 alerts became 22. Two mistakes.
First, the containment check used `base not in candidate.parents`.
That is correct Python but not a form static analysis recognises, so
every path-injection alert stayed and _plugin_dir itself picked up two
more. It now uses os.path.realpath plus os.path.commonpath, which is
both the documented sanitiser shape and stricter than the obvious
alternative: "/x/plugins-evil" startswith "/x/plugins" but is a
different directory, and there is now a test that fails if anyone
swaps commonpath for startswith.
Second, raising ComposerInputError from _plugin_dir and returning
str(exc) added two new py/stack-trace-exposure alerts -- CodeQL flags
exception text reaching a response regardless of the exception's type.
_plugin_dir returns None instead and the three handlers answer with a
fixed literal. There is nothing a caller needs there beyond "that id is
not ok".
Also defines .md\:inline in app.css. composer.html marks five toolbar
button labels `hidden md:inline`, and the class was never defined, so
those labels were hidden at every width and the buttons stayed
icon-only. main's test_web_static_audit.py catches it -- the branch
predates that test, which is why it only surfaced now that CI checks
the merge:
Responsive utility classes referenced in templates but never
defined in app.css (they silently no-op): ['md:inline']
Verified against the merged state -- main's app.css plus this one line,
audited against this branch's templates: 3 passed. The other twelve
classes the audit flags locally are defined on main and are artifacts of
this branch being 54 commits behind.
33 containment tests. Mutation-checked twice: removing the containment
lets eight payloads escape, including /etc/passwd and
plugin/../../../../../../etc/shadow; swapping commonpath for startswith
fails the sibling-prefix test.
Not addressed: three py/stack-trace-exposure alerts on the
_generate_plugin_files handlers. Those return str(exc) for
ComposerInputError, whose seven raise sites are all authored literals
("Author is required.", "Config variable key X is not a valid Python
identifier."). Suppressing them means replacing useful validation
feedback with a generic string, which is a real cost to the user for a
scanner's benefit. Worth a decision rather than a silent downgrade.
The 5 test_web_api.py failures are pre-existing on this branch --
identical counts with these changes stashed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
|
||
|
ChuckBuildsandClaude Opus 5
|
e499efb1f0 |
fix(composer): resolve plugin paths at the filesystem boundary
CodeQL reports 19 alerts against this PR -- 16 high-severity
py/path-injection plus 3 py/stack-trace-exposure -- all in
web_interface/blueprints/composer.py, where a request-supplied plugin_id
reaches Path(plugins_dir) / plugin_id and the result is created, written
to, deleted with shutil.rmtree, and read back.
The path-injection alerts are false positives today. _PLUGIN_ID_RE is
fully anchored and permits only [a-z][a-z0-9-]{0,62}, so every traversal
payload is already rejected; I checked fourteen of them, including
../../etc/passwd, a/../../etc, /etc/passwd and encoded variants, and none
gets past it.
They are worth fixing anyway. The guarantee lived in a regex several
hundred lines from the path building, so relaxing that pattern later --
to allow an underscore, say -- would open a traversal with nothing at the
filesystem boundary to catch it. _plugin_dir() now resolves the candidate
and refuses anything that is not inside plugins_dir, and all three call
sites go through it. That is also the shape static analysis recognises,
which is why sixteen alerts landed on code that was already safe.
The regex anchor moves from $ to \Z. Python's $ also matches just before
a trailing newline, so "myplugin\n" was accepted and would have created a
directory whose name ends in one. Not traversal, but not a name anything
downstream should have to handle.
For the stack-trace exposure: the handlers returned str(exc) for any
ValueError out of _generate_plugin_files. The seven raises there are all
curated, user-facing validation messages, and they now use a
ComposerInputError subclass so they keep reaching the user verbatim. A
ValueError from anywhere else -- json, int(), a library -- is logged with
a traceback and answered generically, since its text can name internal
paths.
Verified: 32 tests covering fourteen traversal payloads and twelve
malformed ids. The key one re-runs every payload with the id pattern
deliberately loosened to allow slashes and dots; removing the containment
check fails it with
these escaped the base with a loosened regex:
[('/etc/passwd', '/etc/passwd'), ('//etc/passwd', '//etc/passwd')]
so the boundary is doing real work rather than shadowing the regex.
The 5 failures in test_web_api.py are unrelated and pre-existing on this
branch -- identical counts with these changes stashed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01STMbQE4YctTacQXfbYqKuW
|
||
|
ChuckBuildsandClaude Sonnet 5
|
cd7e16e58e |
fix(security): validate plugin_id before path construction in /api/install
CodeQL flagged 16 high-severity "path depends on user-provided value" alerts. Investigated each: - install_locally() (/api/install) built a filesystem path from metadata.id without validating it at that point -- it was only implicitly safe because _generate_plugin_files() validates the same field (re-extracted independently) earlier in the same request. That's a real gap: reorder or change that earlier call and it's an exploitable path traversal / arbitrary file write. Fixed by validating plugin_id directly against _PLUGIN_ID_RE at the point the path is built, matching the pattern already used correctly in validate_id() and load_plugin(). - The other 10 flagged locations (serve_font's allowlist check, validate_id, load_plugin and its downstream reads) were already guarded by an explicit check earlier in the same function -- false positives from CodeQL not modeling those as sanitizers. Also fixed 2 of the 5 "stack trace exposed" warnings that were genuine: install_locally() and load_plugin() returned raw OSError/Exception text to the client in a 500 response; now logged server-side with a generic client-facing message. The other 3 (generate_zip/install_locally/ preview_code returning str(ValueError) from _generate_plugin_files) are deliberate, human-authored validation messages, not exception internals -- left as-is. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ |
||
|
ChuckBuildsandClaude Sonnet 5
|
47e3021fc3 |
fix: address Codacy findings in the Composer blueprint
- Dropped a pointless f-string prefix (no placeholders) on the default plugin description. - Replaced two bare except:pass/continue blocks (manifest.json listing, config_schema.json parsing) with a logged warning before falling through to the same skip-this-entry behavior -- same control flow, now visible in logs instead of silent. Skipped as false positives (verified against actual usage, not fixed): - Jinja2 Environment(autoescape=False) -- this env renders manager.py.j2, a Python source-code generator, never HTML; autoescaping would corrupt generated code. Flagged by a generic XSS rule that assumes all Jinja2 environments render HTML. - "Flask route directly returning a formatted string" on _as_rgb_filter -- that's a Jinja *filter* function, not a Flask route. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ |
||
|
ChuckBuildsandClaude Sonnet 5
|
e319540c6e |
feat(web): add Plugin Composer -- visual drag-and-drop plugin builder
Web UI (/composer/) for building a working LEDMatrix plugin without writing Python: drop elements (text, time, date, countdown, scrolling text, bar/waveform, groups, custom config variables) onto a canvas matching the real panel's pixel grid, configure them with live preview, then generate a real plugin (manager.py + manifest.json + config_schema.json) from manager.py.j2 -- downloadable as a ZIP or installed directly. NOTE: composer_bp is not yet registered in web_interface/app.py, so this blueprint is currently inert. Split out of the original chore/dead-code- removal commit, which had accidentally bundled this in alongside unrelated dead-code deletions; app.py registration was not part of that commit either and still needs to be added before this is reachable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KEZK1P1Q1fu5pcuVrkrCFZ |