diff --git a/api/config.py b/api/config.py index b097c35f..4c7f9d22 100644 --- a/api/config.py +++ b/api/config.py @@ -5046,8 +5046,10 @@ def save_settings(settings: dict) -> dict: current_dash.update(_dashboard_plugins) current["dashboard_plugins"] = current_dash for k, v in settings.items(): + # dashboard_plugins is deep-merged above (not a flat allowlisted scalar). if k == "dashboard_plugins": continue + if k in _SETTINGS_ALLOWED_KEYS: if k == "theme": if isinstance(v, str) and v.strip(): pending_theme = v diff --git a/api/plugins.py b/api/plugins.py index ffafb79a..df676a16 100644 --- a/api/plugins.py +++ b/api/plugins.py @@ -15,10 +15,15 @@ Each plugin may have: import json import logging import os +import re from pathlib import Path logger = logging.getLogger(__name__) +# Valid dashboard-plugin name: a safe slug (it becomes a URL path component and +# a settings key). Lowercase alnum + - / _, 1-64 chars, must start with a letter. +_VALID_PLUGIN_NAME = re.compile(r"^[a-z][a-z0-9_-]{0,63}$") + # plugin_name -> manifest dict (as loaded from manifest.json) PLUGIN_MANIFESTS: dict[str, dict] = {} @@ -52,6 +57,13 @@ def load_plugins() -> None: name = manifest.get("name") or entry.name + # Validate the plugin name: it becomes a URL path component + # (/dashboard-plugins//...) and a settings key. Restrict to a safe + # slug so a manifest like name:"../foo" can't make the URL-space ambiguous. + if not _VALID_PLUGIN_NAME.match(str(name)): + logger.warning("Skipping plugin with invalid name %r (must match %s)", name, _VALID_PLUGIN_NAME.pattern) + continue + tab = manifest.get("tab", {}) tab_path = tab.get("path", f"/{name}") diff --git a/tests/test_plugins_panel.py b/tests/test_plugins_panel.py index 234595c3..0795d3a3 100644 --- a/tests/test_plugins_panel.py +++ b/tests/test_plugins_panel.py @@ -373,6 +373,52 @@ class TestPluginAssetIsolationHardening: assert "_dashboard_plugin_enabled" in page_seg +class TestSettingsAllowlistGuard: + """The dashboard_plugins save path must not weaken the settings allowlist. + + Regression for the #2622 hardening: an earlier revision replaced the + `if k in _SETTINGS_ALLOWED_KEYS` guard with `if k == "dashboard_plugins": + continue` and orphaned the validation body, which (a) broke saves for every + other key and (b) let any client-supplied key be written (e.g. password_hash). + """ + + def _isolated(self): + import tempfile + import api.config as cfg + cfg.SETTINGS_FILE = Path(tempfile.mkdtemp()) / "settings.json" + return cfg + + def test_allowlisted_key_persists(self): + cfg = self._isolated() + r = cfg.save_settings({"language": "ru"}) + assert r.get("language") == "ru" + + def test_non_allowlisted_key_rejected(self): + cfg = self._isolated() + r = cfg.save_settings({"password_hash": "INJECTED", "signing_key_evil": "x"}) + assert r.get("password_hash") != "INJECTED" + assert "signing_key_evil" not in r + + def test_dashboard_plugins_deep_merges(self): + cfg = self._isolated() + cfg.save_settings({"dashboard_plugins": {"p1": True}}) + r = cfg.save_settings({"dashboard_plugins": {"p2": True}}) + dp = r.get("dashboard_plugins", {}) + assert dp.get("p1") is True and dp.get("p2") is True + + +class TestPluginNameValidation: + def test_invalid_plugin_name_rejected(self): + import api.plugins as plugins + assert plugins._VALID_PLUGIN_NAME.match("nic-branch-sync") + assert plugins._VALID_PLUGIN_NAME.match("demo_dashboard") + assert not plugins._VALID_PLUGIN_NAME.match("../foo") + assert not plugins._VALID_PLUGIN_NAME.match("/etc/passwd") + assert not plugins._VALID_PLUGIN_NAME.match("UPPER") + assert not plugins._VALID_PLUGIN_NAME.match("") + assert not plugins._VALID_PLUGIN_NAME.match("1leading-digit") + + class TestPluginCollisionDetection: """Tests for plugin name and tab.path collision detection."""