diff --git a/docs/widget-guide.md b/docs/widget-guide.md index 9bc271eb..2ac2a99a 100644 --- a/docs/widget-guide.md +++ b/docs/widget-guide.md @@ -285,7 +285,9 @@ Guidelines: ### Step 1: Create Widget File -Create a JavaScript file in your plugin directory. The recommended location is `widgets/[widget-name].js`: +Create a JavaScript file in your plugin's `widgets/` directory, named +`widgets/[widget-name].js`. The directory is not optional: it is the only +place the core will serve a widget from. ```javascript // Ensure LEDMatrixWidgets registry is available @@ -366,7 +368,29 @@ window.LEDMatrixWidgets.register('my-custom-widget', { }); ``` -### Step 2: Reference Widget in Schema +### Step 2: Declare the Widget in `manifest.json` + +The manifest is the allowlist. A widget is served only if the plugin declares +it, so shipping a file under `widgets/` does not by itself publish it: + +```json +{ + "widgets": [ + { + "name": "my-custom-widget", + "script": "my-custom-widget.js", + "description": "What this widget is for" + } + ] +} +``` + +`name` is what you use in `x-widget` and in the URL. `script` is optional and +defaults to `[name].js`; it must be a plain filename directly inside +`widgets/` (no paths). Both are validated against +`schema/manifest_schema.json`. + +### Step 3: Reference Widget in Schema In your plugin's `config_schema.json`: @@ -383,15 +407,30 @@ In your plugin's `config_schema.json`: } ``` -### Step 3: Widget Loading +### Step 4: Widget Loading -The widget will be automatically loaded when the plugin configuration form is rendered. The system will: +The widget is loaded on demand when the plugin's configuration form renders a +field that references it. The system will: -1. Check if widget is registered in the core registry -2. If not found, attempt to load from plugin directory: `/static/plugin-widgets/[plugin-id]/[widget-name].js` -3. Render the widget using the registered `render` function +1. Check whether the widget is already registered in the core registry. +2. If not, fetch it from `/static/plugin-widgets/[plugin-id]/[widget-name].js`. + That route serves the declared `script` from your plugin's `widgets/` + directory, as `text/javascript`. +3. Render it by calling the `render` function your script registered. -**Note:** Currently, widgets are server-side rendered via Jinja2 templates. Custom widgets registered via the registry will have their handlers available, but full client-side rendering is a future enhancement. +The fetch uses a dynamic `import()`, so the file must parse as an ES module. +A plain IIFE does — modules are strict mode, so avoid sloppy-mode constructs. + +**If the widget fails to load** (not declared, file missing, script throws, or +it never calls `register`), the field falls back to a plain text input holding +the current value. This is deliberate: a broken widget costs the user an +editor, not their configured value. + +**Limitation:** the on-demand path applies to `string`-typed fields (the +default branch of the config-form renderer). Fields typed `object`, `array`, +`boolean`, `integer` or `number`, and fields whose `enum` is set, are +dispatched by the server-side template to its own built-in renderers, so a +plugin-supplied `x-widget` on one of those is ignored today. ## Widget API Reference @@ -497,10 +536,11 @@ See [`web_interface/static/v3/js/widgets/example-color-picker.js`](../web_interf - ✅ Plugin widget loading system implemented **Current Behavior:** -- Widgets are server-side rendered via Jinja2 templates (existing behavior preserved) +- Core widgets are server-side rendered via Jinja2 templates (existing behavior preserved) - Widget handlers are registered and available globally -- Custom widgets can be created and registered -- Full client-side rendering is a future enhancement +- Custom widgets can be created, declared in `manifest.json`, and are served + and rendered on demand for `string`-typed fields +- Plugin widgets on non-string fields are not dispatched yet (see Step 4) **Backwards Compatibility:** - All existing plugins using widgets continue to work without changes diff --git a/schema/manifest_schema.json b/schema/manifest_schema.json index dce337c5..5dd360d8 100644 --- a/schema/manifest_schema.json +++ b/schema/manifest_schema.json @@ -257,6 +257,30 @@ }, "description": "Web UI action definitions" }, + "widgets": { + "type": "array", + "description": "Custom web-UI widgets this plugin provides. Each entry is served at /static/plugin-widgets//.js from the plugin's widgets/ directory; only declared widgets are served. Reference one from config_schema.json with \"x-widget\": \"\".", + "items": { + "type": "object", + "required": ["name"], + "properties": { + "name": { + "type": "string", + "pattern": "^[a-zA-Z0-9_-]{1,64}$", + "description": "Widget name, as used in x-widget and in the URL." + }, + "script": { + "type": "string", + "pattern": "^[a-zA-Z0-9_-]{1,64}\\.js$", + "description": "Filename inside widgets/. Defaults to .js." + }, + "description": { + "type": "string", + "description": "Human-readable summary shown to plugin authors." + } + } + } + }, "ledmatrix_version": { "type": "string", "description": "Deprecated: Use compatible_versions instead. LEDMatrix version this plugin targets" diff --git a/test/web_interface/test_plugin_widget_route.py b/test/web_interface/test_plugin_widget_route.py new file mode 100644 index 00000000..330fb1cf --- /dev/null +++ b/test/web_interface/test_plugin_widget_route.py @@ -0,0 +1,261 @@ +"""The server half of the plugin-supplied widget feature. + +``LEDMatrixWidgets.loadPluginWidget`` (static/v3/js/widgets/plugin-loader.js) +has always fetched ``/static/plugin-widgets//.js``, and +docs/widget-guide.md has always documented that path, but nothing served it -- +so a plugin could declare a widget, ship the file, and still never load it. +soccer-scoreboard has shipped exactly that since August. + +The load-bearing property here is that the manifest is the allowlist. A plugin +directory is attacker-influenced in the sense that matters -- plugins are +user-installed, and the store installs them -- so "serve files from the plugin +directory" would publish everything a plugin ships. Only a widget the manifest +declares is reachable, and only from that plugin's widgets/ directory. +""" + +import json +import sys +from pathlib import Path +from unittest.mock import MagicMock + +import pytest +from flask import Flask + +PROJECT_ROOT = Path(__file__).parent.parent.parent +sys.path.insert(0, str(PROJECT_ROOT)) + +WIDGET_BODY = "(function(){ window.LEDMatrixWidgets.register('custom-leagues', {}); })();\n" + +_UNSET = object() + + +def _make_plugin(plugins_dir, plugin_id="soccer-scoreboard", widgets=None, + files=None): + """Write a plugin directory with a manifest and a widgets/ folder.""" + d = plugins_dir / plugin_id + (d / "widgets").mkdir(parents=True) + manifest = {"id": plugin_id, "name": plugin_id, "version": "1.0.0"} + if widgets is not None: + manifest["widgets"] = widgets + (d / "manifest.json").write_text(json.dumps(manifest), encoding="utf-8") + for name, body in (files or {}).items(): + (d / "widgets" / name).write_text(body, encoding="utf-8") + return d + + +@pytest.fixture +def make_client(tmp_path): + """Build a test client whose plugin manager points at a temp plugins dir. + + pages_v3 is a module-level Blueprint singleton shared across the test + process, so the original plugin_manager is restored on teardown. + """ + from web_interface.blueprints import pages_v3 as pv + + original_pm = getattr(pv.pages_v3, "plugin_manager", None) + + def _build(plugins_dir=None, plugin_manager=_UNSET): + base = PROJECT_ROOT / "web_interface" + app = Flask(__name__, + template_folder=str(base / "templates"), + static_folder=str(base / "static")) + app.config["TESTING"] = True + + if plugin_manager is _UNSET: + plugin_manager = MagicMock() + plugin_manager.plugins_dir = str(plugins_dir or tmp_path) + pv.pages_v3.plugin_manager = plugin_manager + + app.register_blueprint(pv.pages_v3, url_prefix="") + return app.test_client() + + try: + yield _build + finally: + pv.pages_v3.plugin_manager = original_pm + + +URL = "/static/plugin-widgets/{}/{}.js" + + +class TestDeclaredWidgetIsServed: + def test_a_declared_widget_is_served(self, tmp_path, make_client): + _make_plugin(tmp_path, + widgets=[{"name": "custom-leagues", + "script": "custom-leagues.js"}], + files={"custom-leagues.js": WIDGET_BODY}) + r = make_client().get(URL.format("soccer-scoreboard", "custom-leagues")) + assert r.status_code == 200 + assert r.get_data(as_text=True) == WIDGET_BODY + + def test_it_is_served_as_javascript(self, tmp_path, make_client): + """The loader uses dynamic import(); a wrong MIME type is refused.""" + _make_plugin(tmp_path, + widgets=[{"name": "custom-leagues", + "script": "custom-leagues.js"}], + files={"custom-leagues.js": WIDGET_BODY}) + r = make_client().get(URL.format("soccer-scoreboard", "custom-leagues")) + assert "javascript" in r.headers["Content-Type"] + + def test_script_defaults_to_the_widget_name(self, tmp_path, make_client): + _make_plugin(tmp_path, widgets=[{"name": "custom-leagues"}], + files={"custom-leagues.js": WIDGET_BODY}) + r = make_client().get(URL.format("soccer-scoreboard", "custom-leagues")) + assert r.status_code == 200 + + def test_the_ledmatrix_prefix_fallback_resolves(self, tmp_path, make_client): + """PluginManager resolves 'music' to 'ledmatrix-music'; so must this.""" + _make_plugin(tmp_path, plugin_id="ledmatrix-music", + widgets=[{"name": "deck", "script": "deck.js"}], + files={"deck.js": WIDGET_BODY}) + r = make_client().get(URL.format("music", "deck")) + assert r.status_code == 200 + + +class TestTheManifestIsTheAllowlist: + def test_an_undeclared_file_in_widgets_is_not_served(self, tmp_path, make_client): + """The whole point: shipping a file does not publish it.""" + _make_plugin(tmp_path, widgets=[], + files={"secrets.js": "const KEY='hunter2';"}) + r = make_client().get(URL.format("soccer-scoreboard", "secrets")) + assert r.status_code == 404 + assert "hunter2" not in r.get_data(as_text=True) + + def test_a_manifest_with_no_widgets_key_serves_nothing(self, tmp_path, make_client): + _make_plugin(tmp_path, files={"anything.js": WIDGET_BODY}) + r = make_client().get(URL.format("soccer-scoreboard", "anything")) + assert r.status_code == 404 + + def test_a_declared_widget_whose_file_is_missing_is_404(self, tmp_path, make_client): + _make_plugin(tmp_path, widgets=[{"name": "ghost", "script": "ghost.js"}]) + r = make_client().get(URL.format("soccer-scoreboard", "ghost")) + assert r.status_code == 404 + + def test_a_malformed_manifest_serves_nothing(self, tmp_path, make_client): + d = tmp_path / "broken" + (d / "widgets").mkdir(parents=True) + (d / "manifest.json").write_text("{not json", encoding="utf-8") + (d / "widgets" / "w.js").write_text(WIDGET_BODY, encoding="utf-8") + assert make_client().get(URL.format("broken", "w")).status_code == 404 + + +class TestPathTraversal: + @pytest.mark.parametrize("plugin_id", ["../etc", "..%2f..", "a/b", "a\\b", ""]) + def test_a_hostile_plugin_id_never_reaches_the_filesystem( + self, plugin_id, tmp_path, make_client): + r = make_client().get(URL.format(plugin_id, "custom-leagues")) + assert r.status_code in (400, 404), r.status_code + + @pytest.mark.parametrize("widget", ["../manifest", "..%2fsecret", "a/b"]) + def test_a_hostile_widget_name_never_reaches_the_filesystem( + self, widget, tmp_path, make_client): + _make_plugin(tmp_path, widgets=[{"name": "custom-leagues"}], + files={"custom-leagues.js": WIDGET_BODY}) + r = make_client().get(URL.format("soccer-scoreboard", widget)) + assert r.status_code in (400, 404), r.status_code + + def test_a_manifest_cannot_escape_the_widgets_directory(self, tmp_path, make_client): + """A hostile manifest is the traversal vector the URL allowlist can't + cover: the script name comes from the plugin, not the request.""" + (tmp_path / "loot.js").write_text("const KEY='hunter2';", encoding="utf-8") + _make_plugin(tmp_path, + widgets=[{"name": "evil", "script": "../../loot.js"}]) + r = make_client().get(URL.format("soccer-scoreboard", "evil")) + assert r.status_code == 404 + assert "hunter2" not in r.get_data(as_text=True) + + +class TestDegradation: + def test_an_unknown_plugin_is_404(self, tmp_path, make_client): + assert make_client().get(URL.format("nope", "w")).status_code == 404 + + def test_no_plugin_manager_is_503(self, make_client): + r = make_client(plugin_manager=None).get(URL.format("any", "w")) + assert r.status_code == 503 + + +def _schema_with_widget(widget_name): + return { + "type": "object", + "properties": { + "enabled": {"type": "boolean", "default": False}, + "leagues": {"type": "string", "default": "eng.1", + "x-widget": widget_name}, + }, + } + + +@pytest.fixture +def config_form(tmp_path): + """Render a plugin's config partial with a temp plugin on disk.""" + from web_interface.blueprints import pages_v3 as pv + + orig_pm = getattr(pv.pages_v3, "plugin_manager", None) + orig_cm = getattr(pv.pages_v3, "config_manager", None) + + def _render(plugin_id="soccer-scoreboard", schema=None, widgets=None, + files=None): + d = _make_plugin(tmp_path, plugin_id, widgets=widgets, files=files) + (d / "config_schema.json").write_text( + json.dumps(schema or {"type": "object", "properties": {}}), + encoding="utf-8") + + pm = MagicMock() + pm.plugins_dir = str(tmp_path) + pm.get_plugin_info.return_value = {"id": plugin_id, "name": plugin_id} + pm.get_plugin.return_value = None + pv.pages_v3.plugin_manager = pm + + cm = MagicMock() + cm.load_config.return_value = {plugin_id: {"enabled": True}} + pv.pages_v3.config_manager = cm + + base = PROJECT_ROOT / "web_interface" + app = Flask(__name__, + template_folder=str(base / "templates"), + static_folder=str(base / "static")) + app.config["TESTING"] = True + app.register_blueprint(pv.pages_v3, url_prefix="") + return app.test_client().get(f"/partials/plugin-config/{plugin_id}") + + try: + yield _render + finally: + pv.pages_v3.plugin_manager = orig_pm + pv.pages_v3.config_manager = orig_cm + + +class TestTheFormRequestsPluginWidgets: + """Without this the feature is still dead: the route can serve a widget, + but nothing ever asks for one. The server-side form only knows a hardcoded + list of core widget names, so a plugin's own x-widget fell through to a + plain text input and was never fetched.""" + + def test_an_unknown_widget_name_triggers_a_plugin_load(self, config_form): + r = config_form(schema=_schema_with_widget("custom-leagues"), + widgets=[{"name": "custom-leagues"}], + files={"custom-leagues.js": WIDGET_BODY}) + assert r.status_code == 200 + body = r.get_data(as_text=True) + assert "ensureWidget" in body + assert '"custom-leagues"' in body + + def test_the_text_input_remains_as_the_fallback(self, config_form): + """A widget that fails to load must not cost the user their value.""" + r = config_form(schema=_schema_with_widget("custom-leagues"), + widgets=[{"name": "custom-leagues"}], + files={"custom-leagues.js": WIDGET_BODY}) + body = r.get_data(as_text=True) + assert 'name="leagues"' in body + assert 'value="eng.1"' in body + + def test_a_plain_string_field_asks_for_no_widget(self, config_form): + r = config_form(schema={"type": "object", "properties": { + "leagues": {"type": "string", "default": "eng.1"}}}) + assert "ensureWidget" not in r.get_data(as_text=True) + + def test_a_core_widget_does_not_take_the_plugin_path(self, config_form): + r = config_form(schema=_schema_with_widget("font-selector")) + body = r.get_data(as_text=True) + assert "ensureWidget" not in body + assert "LEDMatrixWidgets.get('font-selector')" in body diff --git a/web_interface/blueprints/pages_v3.py b/web_interface/blueprints/pages_v3.py index 958683c8..b6a20c01 100644 --- a/web_interface/blueprints/pages_v3.py +++ b/web_interface/blueprints/pages_v3.py @@ -12,6 +12,8 @@ from pathlib import Path # Strict allowlists for URL-derived values used in path and script operations. _SAFE_PLUGIN_ID_RE = re.compile(r'^[a-zA-Z0-9_-]{1,64}$') _SAFE_WEB_UI_FILE_RE = re.compile(r'^[a-zA-Z0-9_-]{1,64}\.html$') +_SAFE_WIDGET_NAME_RE = re.compile(r'^[a-zA-Z0-9_-]{1,64}$') +_SAFE_WIDGET_SCRIPT_RE = re.compile(r'^[a-zA-Z0-9_-]{1,64}\.js$') from src.web_interface.secret_helpers import mask_secret_fields logger = logging.getLogger(__name__) @@ -358,6 +360,122 @@ def serve_plugin_web_ui(plugin_id, filename): logger.error('Error serving plugin web_ui %s/%s', plugin_id, filename, exc_info=True) return 'Error serving file', 500, {'Content-Type': 'text/plain'} + +def _plugin_dir_for(safe_id): + """Resolve a sanitised plugin id to its directory, or None. + + Mirrors serve_plugin_web_ui: containment-guarded against the configured + plugins directory, with PluginManager's ``ledmatrix-`` prefix fallback. + """ + plugins_base = Path(pages_v3.plugin_manager.plugins_dir).resolve() + plugin_dir = (plugins_base / safe_id).resolve() + plugin_dir.relative_to(plugins_base) # containment guard + + if not plugin_dir.exists(): + alt_id = os.path.basename(f'ledmatrix-{safe_id}') + alt = (plugins_base / alt_id).resolve() + try: + alt.relative_to(plugins_base) + plugin_dir = alt + except ValueError: + pass + return plugin_dir + + +def _declared_widget_script(plugin_dir, widget_name): + """The script filename a plugin's manifest declares for ``widget_name``. + + The manifest is the allowlist: only a widget the plugin actually declares + can be served, so this route never exposes arbitrary files under the + plugin directory even though the directory itself is attacker-influenced + (plugins are user-installed). Returns None when the widget is not + declared, the manifest is unreadable, or the declared script name is not + a plain ``.js`` basename. + """ + manifest_path = plugin_dir / 'manifest.json' + try: + with open(manifest_path, 'r', encoding='utf-8') as f: + manifest = json.load(f) + except (OSError, ValueError): + return None + if not isinstance(manifest, dict): + return None + + for entry in manifest.get('widgets') or (): + if not isinstance(entry, dict): + continue + if entry.get('name') != widget_name: + continue + script = entry.get('script') or f'{widget_name}.js' + if not isinstance(script, str) or not _SAFE_WIDGET_SCRIPT_RE.match(script): + return None + return script + return None + + +@pages_v3.route('/static/plugin-widgets//.js') +def serve_plugin_widget(plugin_id, widget_name): + """Serve a plugin-declared widget script from its ``widgets/`` directory. + + This is the server half of ``LEDMatrixWidgets.loadPluginWidget`` (see + static/v3/js/widgets/plugin-loader.js), which fetches exactly this path. + The loader uses a dynamic ``import()``, so the response must carry a + JavaScript MIME type or the browser refuses the module. + + The route is deliberately narrower than the plugin directory: a script is + served only when the plugin's own manifest declares a widget by that name, + so installing a plugin does not publish everything it ships. + """ + if not _SAFE_PLUGIN_ID_RE.match(plugin_id): + return 'Invalid plugin ID', 400, {'Content-Type': 'text/plain'} + if not _SAFE_WIDGET_NAME_RE.match(widget_name): + return 'Invalid widget name', 400, {'Content-Type': 'text/plain'} + + # os.path.basename() is the CodeQL-recognised path sanitizer used + # throughout this codebase; the allowlists above already exclude + # separators, this breaks the taint chain. + safe_id = os.path.basename(plugin_id) + safe_widget = os.path.basename(widget_name) + if not safe_id or not safe_widget: + return 'Invalid path component', 400, {'Content-Type': 'text/plain'} + + if not pages_v3.plugin_manager: + return 'Plugin manager not available', 503, {'Content-Type': 'text/plain'} + + try: + plugin_dir = _plugin_dir_for(safe_id) + if not plugin_dir.exists(): + return 'Not found', 404, {'Content-Type': 'text/plain'} + + script = _declared_widget_script(plugin_dir, safe_widget) + if script is None: + # Undeclared is a 404 rather than a 403: whether a plugin happens + # to ship an undeclared file is not something to confirm. + return 'Not found', 404, {'Content-Type': 'text/plain'} + + widgets_dir = (plugin_dir / 'widgets').resolve() + script_path = (widgets_dir / os.path.basename(script)).resolve() + script_path.relative_to(widgets_dir) # second containment guard + + if not script_path.is_file(): + return 'Not found', 404, {'Content-Type': 'text/plain'} + + body = script_path.read_text(encoding='utf-8') + return body, 200, { + 'Content-Type': 'text/javascript; charset=utf-8', + # Plugin updates replace this file in place; revalidate so a + # stale widget cannot outlive the plugin version that shipped it. + 'Cache-Control': 'no-cache', + } + + except ValueError: + return 'Forbidden', 403, {'Content-Type': 'text/plain'} + except Exception: + logger.error('Error serving plugin widget %s/%s', plugin_id, widget_name, + exc_info=True) + return 'Error serving file', 500, {'Content-Type': 'text/plain'} + + def _load_overview_partial(): """Load overview partial with system stats""" try: diff --git a/web_interface/static/v3/js/widgets/README.md b/web_interface/static/v3/js/widgets/README.md index c2ecbcd1..be1f2189 100644 --- a/web_interface/static/v3/js/widgets/README.md +++ b/web_interface/static/v3/js/widgets/README.md @@ -280,14 +280,40 @@ In your plugin's `config_schema.json`: } ``` -### Step 3: Widget Loading +### Step 3: Declare the Widget in `manifest.json` -The widget will be automatically loaded when the plugin configuration form is rendered. The system will: +The manifest is the allowlist -- a widget is served only if the plugin declares +it, so shipping a file under `widgets/` does not by itself publish it: -1. Check if widget is registered in the core registry -2. If not found, attempt to load from plugin directory: `/static/plugin-widgets/[plugin-id]/[widget-name].js` +```json +{ + "widgets": [ + { "name": "my-custom-widget", "script": "my-custom-widget.js" } + ] +} +``` + +`script` is optional and defaults to `[name].js`. It must be a plain filename +directly inside the plugin's `widgets/` directory. + +### Step 4: Widget Loading + +The widget is loaded on demand when the config form renders a field that +references it. The system will: + +1. Check if the widget is registered in the core registry +2. If not, fetch `/static/plugin-widgets/[plugin-id]/[widget-name].js`, which + serves the declared script from the plugin's `widgets/` directory 3. Render the widget using the registered `render` function +The fetch is a dynamic `import()`, so the file must parse as an ES module (a +plain IIFE does). If anything fails, the field falls back to a plain text input +holding the current value, so a broken widget never costs the user their +configured value. + +Only `string`-typed fields take this path today; see `docs/widget-guide.md` +for the full details and limitations. + ## Widget API Reference ### Widget Definition Object diff --git a/web_interface/templates/v3/partials/plugin_config.html b/web_interface/templates/v3/partials/plugin_config.html index 9e5f3707..190d85df 100644 --- a/web_interface/templates/v3/partials/plugin_config.html +++ b/web_interface/templates/v3/partials/plugin_config.html @@ -872,6 +872,65 @@ name="{{ full_key }}" value="{{ str_value }}" class="form-input w-full rounded-md border-gray-300 shadow-sm focus:border-blue-500 focus:ring-blue-500 bg-white text-black placeholder:text-gray-500"> + {% if str_widget %} + {# An x-widget the core does not ship may be supplied by the + plugin itself (manifest "widgets", served from its widgets/ + directory). Ask the loader for it; the text input above is + the fallback and stays put unless the widget really renders, + so a missing or broken widget degrades to an editable field + rather than dropping the value on save. #} +
+ + {% endif %} {% endif %} {% endif %}