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
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
