fix(#2622): restore settings allowlist guard (Opus HALT) + validate plugin name

CRITICAL (Opus HALT on prior commit): the PR's edit to save_settings() replaced
'if k in _SETTINGS_ALLOWED_KEYS' with 'if k=="dashboard_plugins": continue' and
orphaned the whole validation body under the continue. Effects: (a) settings save
broken for every key except dashboard_plugins; (b) the allowlist security boundary
gone -> any client key (password_hash, signing_key_*) became settable. Restored the
guard + correct indentation; dashboard_plugins handled by the deep-merge above.
Verified in-process: language persists, password_hash/signing_key injection
rejected, dashboard_plugins still deep-merges.

Also (Opus SHOULD-FIX #3): validate plugin name against ^[a-z][a-z0-9_-]{0,63}$
so a manifest name like '../foo' can't make the URL-space ambiguous.

Regression tests added for both (the allowlist bug had ZERO coverage).

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

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/<name>/...) 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}")
 

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