Harden module installation and imports

alembic/dev_versions/2e3f4a5b6c7d_namespace_platform_tables.py

@@ -30,10 +30,14 @@ _TABLE_RENAMES = (
     ("governance_templates", "admin_governance_templates"),
     ("governance_template_assignments", "admin_governance_template_assignments"),
 )
+_KNOWN_TABLE_NAMES = {name for pair in _TABLE_RENAMES for name in pair}
 
 
 def _row_count(bind: sa.Connection, table_name: str) -> int:
-    return int(bind.execute(sa.text(f"SELECT COUNT(*) FROM {table_name}")).scalar_one())
+    if table_name not in _KNOWN_TABLE_NAMES:
+        raise RuntimeError(f"Unexpected table name: {table_name}")
+    quoted = bind.dialect.identifier_preparer.quote(table_name)
+    return int(bind.execute(sa.text(f"SELECT COUNT(*) FROM {quoted}")).scalar_one())  # nosemgrep: python.sqlalchemy.security.audit.avoid-sqlalchemy-text.avoid-sqlalchemy-text
 
 
 def _rename_tables(renames: tuple[tuple[str, str], ...]) -> None:

alembic/dev_versions/4f2a9c8e7b6d_rename_tenancy_tenants_to_core_scopes.py

@@ -19,11 +19,14 @@ LEGACY_SCOPE_TABLE = "tenancy_tenants"
 CORE_SCOPE_TABLE = "core_scopes"
 LEGACY_SLUG_INDEX = "ix_tenancy_tenants_slug"
 CORE_SLUG_INDEX = "ix_core_scopes_slug"
+_KNOWN_SCOPE_TABLES = {LEGACY_SCOPE_TABLE, CORE_SCOPE_TABLE}
 
 
 def _row_count(bind: sa.Connection, table_name: str) -> int:
+    if table_name not in _KNOWN_SCOPE_TABLES:
+        raise RuntimeError(f"Unexpected scope table name: {table_name}")
     quoted = bind.dialect.identifier_preparer.quote(table_name)
-    return int(bind.execute(sa.text(f"SELECT COUNT(*) FROM {quoted}")).scalar_one())
+    return int(bind.execute(sa.text(f"SELECT COUNT(*) FROM {quoted}")).scalar_one())  # nosemgrep: python.sqlalchemy.security.audit.avoid-sqlalchemy-text.avoid-sqlalchemy-text
 
 
 def _scope_tables(bind: sa.Connection) -> set[str]:

alembic/dev_versions/9d0e1f2a3b4c_system_governance_templates.py

@@ -16,6 +16,7 @@ revision = "9d0e1f2a3b4c"
 down_revision = "8c9d0e1f2a3b"
 branch_labels = None
 depends_on = None
+_RECONCILE_CREATE_ALL_TABLES = ("admin_governance_template_assignments", "admin_governance_templates", "core_system_settings")
 
 
 def _now() -> datetime:
@@ -28,9 +29,10 @@ def upgrade() -> None:
     tables = set(inspector.get_table_names())
 
     # Reconcile only the empty create_all shape for the newly introduced tables.
-    for table_name in ("admin_governance_template_assignments", "admin_governance_templates", "core_system_settings"):
+    for table_name in _RECONCILE_CREATE_ALL_TABLES:
         if table_name in tables:
-            count = bind.execute(sa.text(f"SELECT COUNT(*) FROM {table_name}")).scalar_one()
+            quoted = bind.dialect.identifier_preparer.quote(table_name)
+            count = bind.execute(sa.text(f"SELECT COUNT(*) FROM {quoted}")).scalar_one()  # nosemgrep: python.sqlalchemy.security.audit.avoid-sqlalchemy-text.avoid-sqlalchemy-text
             if count:
                 raise RuntimeError(f"Cannot reconcile non-empty create_all table {table_name}")
             op.drop_table(table_name)

alembic/dev_versions/b1c2d3e4f5a6_audit_scope_and_tenant_owner_selection.py

@@ -49,7 +49,7 @@ def upgrade() -> None:
     placeholders = ", ".join(f":action_{index}" for index, _ in enumerate(SYSTEM_ACTIONS))
     params = {f"action_{index}": action for index, action in enumerate(SYSTEM_ACTIONS)}
     bind.execute(
-        sa.text(f"UPDATE audit_log SET scope = 'system' WHERE action IN ({placeholders})"),
+        sa.text(f"UPDATE audit_log SET scope = 'system' WHERE action IN ({placeholders})"),  # nosemgrep: python.sqlalchemy.security.audit.avoid-sqlalchemy-text.avoid-sqlalchemy-text
         params,
     )
     bind.execute(sa.text("UPDATE audit_log SET scope = 'tenant' WHERE scope IS NULL OR scope NOT IN ('tenant', 'system')"))

src/govoplan_core/core/configuration_packages.py

@@ -690,7 +690,7 @@ def _read_trusted_keys_url(url: str) -> str:
         raise ValueError("Trusted configuration catalog key URL must use http:// or https://.")
     cache_path = _configured_trusted_keys_cache_path()
     try:
-        with urllib.request.urlopen(url, timeout=15) as response:
+        with urllib.request.urlopen(url, timeout=15) as response:  # noqa: S310 - validated configuration key HTTP(S) URL.  # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected
             body = response.read().decode("utf-8")
         if cache_path is not None:
             cache_path.parent.mkdir(parents=True, exist_ok=True)
@@ -714,7 +714,7 @@ def _read_catalog_payload_with_metadata(source: Path | str | None) -> tuple[obje
         return {"packages": []}, metadata
     if isinstance(source, str) and _is_http_url(source):
         try:
-            with urllib.request.urlopen(source, timeout=15) as response:
+            with urllib.request.urlopen(source, timeout=15) as response:  # noqa: S310 - validated configuration catalog HTTP(S) URL.  # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected
                 body = response.read().decode("utf-8")
             if cache_path is not None:
                 cache_path.parent.mkdir(parents=True, exist_ok=True)

src/govoplan_core/core/module_installer.py

@@ -11,6 +11,7 @@ import json
 import os
 from pathlib import Path
 import re
+import shlex
 import shutil
 import sqlite3
 import subprocess
@@ -2611,7 +2612,7 @@ def _run_restart_command(command: str) -> dict[str, object]:
             "stdout": "",
             "stderr": "",
         }
-    result = subprocess.run(command, shell=True, text=True, capture_output=True, check=False)
+    result = _run_operator_command(command)
     return {
         "command": command,
         "return_code": result.returncode,
@@ -2685,13 +2686,16 @@ def _python_package_name_from_ref(value: str) -> str | None:
 
 
 def _wait_for_health_url(url: str, *, timeout_seconds: float, interval_seconds: float) -> dict[str, object]:
+    parsed = urllib.parse.urlparse(url)
+    if parsed.scheme not in {"http", "https"} or not parsed.netloc or parsed.username or parsed.password:
+        return {"ok": False, "attempts": 0, "error": "health URL must be an absolute HTTP(S) URL without embedded credentials"}
     deadline = time.monotonic() + max(timeout_seconds, 0.1)
     attempts = 0
     last_error = ""
     while time.monotonic() < deadline:
         attempts += 1
         try:
-            with urllib.request.urlopen(url, timeout=min(max(interval_seconds, 0.5), 10.0)) as response:
+            with urllib.request.urlopen(url, timeout=min(max(interval_seconds, 0.5), 10.0)) as response:  # noqa: S310 - validated module health HTTP(S) URL.  # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected
                 status = int(getattr(response, "status", 0))
                 if 200 <= status < 400:
                     return {"ok": True, "status": status, "attempts": attempts}
@@ -3217,15 +3221,53 @@ def _run_database_hook(
         if pgtools_url:
             env["GOVOPLAN_DATABASE_URL_PGTOOLS"] = pgtools_url
         env.setdefault("DATABASE_URL", database_url)
+    return _run_operator_command(command, cwd=run_dir, env=env)
+
+
+def _run_operator_command(
+    command: str,
+    *,
+    cwd: Path | None = None,
+    env: Mapping[str, str] | None = None,
+) -> subprocess.CompletedProcess[str]:
+    try:
+        argv = _operator_command_argv(command)
+    except ValueError as exc:
+        return subprocess.CompletedProcess(args=command, returncode=2, stdout="", stderr=str(exc))
+    try:
         return subprocess.run(
-        command,
+            argv,
-        cwd=run_dir,
+            cwd=cwd,
-        env=env,
+            env=dict(env) if env is not None else None,
-        shell=True,
             text=True,
             capture_output=True,
             check=False,
         )
+    except OSError as exc:
+        return subprocess.CompletedProcess(args=argv, returncode=127, stdout="", stderr=str(exc))
+
+
+def _operator_command_argv(command: str) -> tuple[str, ...]:
+    try:
+        argv = tuple(shlex.split(command))
+    except ValueError as exc:
+        raise ValueError(f"Invalid command syntax: {exc}") from exc
+    if not argv:
+        raise ValueError("Command is empty")
+    if any(_operator_command_part_uses_shell(part) for part in argv):
+        raise ValueError("Shell operators, substitutions, and redirects are not supported in installer hooks")
+    if _looks_like_env_assignment(argv[0]):
+        raise ValueError("Environment assignments must be configured through installer hook environment, not shell syntax")
+    return argv
+
+
+def _operator_command_part_uses_shell(value: str) -> bool:
+    return value in {"|", "||", "&", "&&", ";", "<", ">", ">>", "2>", "2>>"} or "$(" in value or "`" in value
+
+
+def _looks_like_env_assignment(value: str) -> bool:
+    key, separator, _rest = value.partition("=")
+    return bool(separator) and bool(re.match(r"^[A-Za-z_][A-Za-z0-9_]*$", key))
 
 
 def _database_url_for_pgtools(database_url: str) -> str | None:

src/govoplan_core/core/module_license.py

@@ -8,6 +8,7 @@ import os
 from pathlib import Path
 from typing import Any
 import urllib.error
+import urllib.parse
 import urllib.request
 
 from cryptography.exceptions import InvalidSignature
@@ -257,11 +258,12 @@ def _configured_trusted_keys_cache_path() -> Path | None:
 
 
 def _read_trusted_keys_url(url: str) -> str:
-    if not url.startswith(("https://", "http://")):
+    parsed = urllib.parse.urlparse(url)
-        raise ValueError("Trusted license key URL must use http:// or https://.")
+    if parsed.scheme not in {"http", "https"} or not parsed.netloc or parsed.username or parsed.password:
+        raise ValueError("Trusted license key URL must be an absolute HTTP(S) URL without embedded credentials.")
     cache_path = _configured_trusted_keys_cache_path()
     try:
-        with urllib.request.urlopen(url, timeout=15) as response:
+        with urllib.request.urlopen(url, timeout=15) as response:  # noqa: S310 - validated license key HTTP(S) URL.  # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected
             body = response.read().decode("utf-8")
         if cache_path is not None:
             cache_path.parent.mkdir(parents=True, exist_ok=True)

src/govoplan_core/core/module_package_catalog.py

@@ -354,7 +354,7 @@ def _read_trusted_keys_url(url: str) -> str:
         raise ValueError("Trusted catalog key URL must use http:// or https://.")
     cache_path = _configured_trusted_keys_cache_path()
     try:
-        with urllib.request.urlopen(url, timeout=15) as response:
+        with urllib.request.urlopen(url, timeout=15) as response:  # noqa: S310 - validated catalog key HTTP(S) URL.  # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected
             body = response.read().decode("utf-8")
         if cache_path is not None:
             cache_path.parent.mkdir(parents=True, exist_ok=True)
@@ -421,7 +421,7 @@ def _read_catalog_url(url: str) -> str:
 def _read_catalog_url_with_metadata(url: str) -> tuple[str, bool]:
     cache_path = _configured_catalog_cache_path()
     try:
-        with urllib.request.urlopen(url, timeout=15) as response:
+        with urllib.request.urlopen(url, timeout=15) as response:  # noqa: S310 - validated module catalog HTTP(S) URL.  # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected
             body = response.read().decode("utf-8")
         if cache_path is not None:
             cache_path.parent.mkdir(parents=True, exist_ok=True)
@@ -935,7 +935,8 @@ def _catalog_source_type(source: Path | str | None) -> str | None:
 
 
 def _is_http_url(value: str) -> bool:
-    return value.startswith(("https://", "http://"))
+    parsed = urllib.parse.urlparse(value)
+    return parsed.scheme in {"http", "https"} and bool(parsed.netloc) and not parsed.username and not parsed.password
 
 
 def _invalid_catalog_result(

src/govoplan_core/db/migrations.py

@@ -5,6 +5,7 @@ from dataclasses import dataclass, replace
 import json
 import os
 from pathlib import Path
+import re
 from typing import Any
 
 from alembic import command
@@ -44,6 +45,7 @@ MIGRATION_TASK_PHASES = (*PRE_MIGRATION_TASK_PHASES, *POST_MIGRATION_TASK_PHASES
 MIGRATION_TRACK_RELEASE = "release"
 MIGRATION_TRACK_DEV = "dev"
 MIGRATION_TRACKS = (MIGRATION_TRACK_RELEASE, MIGRATION_TRACK_DEV)
+_SQL_IDENTIFIER_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$")
 
 _NAMESPACE_TABLE_RENAMES = (
     ("tenants", "tenancy_tenants"),
@@ -521,19 +523,25 @@ def _backfill_user_lock_state_for_create_all_schema(database_url: str) -> None:
 
 
 def _row_count(connection, table_name: str) -> int:
-    quoted = connection.dialect.identifier_preparer.quote(table_name)
+    quoted = _quoted_table_name(connection, table_name)
-    return int(connection.execute(text(f"SELECT COUNT(*) FROM {quoted}")).scalar_one())
+    return int(connection.execute(text(f"SELECT COUNT(*) FROM {quoted}")).scalar_one())  # nosemgrep: python.sqlalchemy.security.audit.avoid-sqlalchemy-text.avoid-sqlalchemy-text
 
 
 def _drop_table(connection, table_name: str) -> None:
-    quoted = connection.dialect.identifier_preparer.quote(table_name)
+    quoted = _quoted_table_name(connection, table_name)
-    connection.execute(text(f"DROP TABLE {quoted}"))
+    connection.execute(text(f"DROP TABLE {quoted}"))  # nosemgrep: python.sqlalchemy.security.audit.avoid-sqlalchemy-text.avoid-sqlalchemy-text
 
 
 def _rename_table(connection, old_name: str, new_name: str) -> None:
-    quoted_old = connection.dialect.identifier_preparer.quote(old_name)
+    quoted_old = _quoted_table_name(connection, old_name)
-    quoted_new = connection.dialect.identifier_preparer.quote(new_name)
+    quoted_new = _quoted_table_name(connection, new_name)
-    connection.execute(text(f"ALTER TABLE {quoted_old} RENAME TO {quoted_new}"))
+    connection.execute(text(f"ALTER TABLE {quoted_old} RENAME TO {quoted_new}"))  # nosemgrep: python.sqlalchemy.security.audit.avoid-sqlalchemy-text.avoid-sqlalchemy-text
+
+
+def _quoted_table_name(connection, table_name: str) -> str:
+    if not _SQL_IDENTIFIER_RE.fullmatch(table_name):
+        raise ValueError(f"Unsafe table identifier: {table_name!r}")
+    return connection.dialect.identifier_preparer.quote(table_name)
 
 
 def _reconcile_scope_table_names(connection, tables: set[str]) -> bool:

src/govoplan_core/server/config.py

@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import os
+import re
 from collections.abc import Callable, Iterable, Mapping, Sequence
 from dataclasses import dataclass, field
 from importlib import import_module
@@ -15,6 +16,10 @@ from govoplan_core.server.fastapi import LifespanFactory
 ManifestFactory = Callable[[], ModuleManifest]
 RouterEnabled = Callable[[object | None, PlatformRegistry], bool]
 AppConfigurator = Callable[[FastAPI, PlatformRegistry, object | None], None]
+DEFAULT_TRUSTED_IMPORT_PREFIXES = ("govoplan_core", "govoplan_", "app")
+TRUSTED_IMPORT_PREFIXES_ENV = "GOVOPLAN_TRUSTED_IMPORT_PREFIXES"
+_MODULE_PATH_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*(?:\.[A-Za-z_][A-Za-z0-9_]*)*$")
+_ATTRIBUTE_PATH_RE = re.compile(r"^[A-Za-z][A-Za-z0-9_]*(?:\.[A-Za-z][A-Za-z0-9_]*)*$")
 
 
 @dataclass(frozen=True, slots=True)
@@ -49,16 +54,52 @@ class GovoplanServerConfig:
 
 
 def import_object(path: str) -> Any:
-    module_name, separator, attribute = path.partition(":")
+    module_name, attribute = validate_object_path(path)
-    if not separator:
+    _validate_trusted_import_prefix(module_name, trusted_import_prefixes())
-        raise ValueError(f"Object path must use module:attribute syntax: {path!r}")
+    module = import_module(module_name)  # nosemgrep: python.lang.security.audit.non-literal-import.non-literal-import
-    module = import_module(module_name)
     value: Any = module
     for part in attribute.split("."):
         value = getattr(value, part)
     return value
 
 
+def validate_object_path(path: str) -> tuple[str, str]:
+    module_name, separator, attribute = path.strip().partition(":")
+    if not separator:
+        raise ValueError(f"Object path must use module:attribute syntax: {path!r}")
+    if not _MODULE_PATH_RE.fullmatch(module_name):
+        raise ValueError(f"Object path module is not a valid Python module path: {path!r}")
+    if not _ATTRIBUTE_PATH_RE.fullmatch(attribute) or any(part.startswith("_") for part in attribute.split(".")):
+        raise ValueError(f"Object path attribute is not a public attribute path: {path!r}")
+    return module_name, attribute
+
+
+def trusted_import_prefixes() -> tuple[str, ...]:
+    configured = os.getenv(TRUSTED_IMPORT_PREFIXES_ENV, "").strip()
+    if not configured:
+        return DEFAULT_TRUSTED_IMPORT_PREFIXES
+    prefixes = tuple(item.strip() for item in configured.split(",") if item.strip())
+    return prefixes or DEFAULT_TRUSTED_IMPORT_PREFIXES
+
+
+def _validate_trusted_import_prefix(module_name: str, prefixes: Iterable[str]) -> None:
+    if any(_module_matches_prefix(module_name, prefix) for prefix in prefixes):
+        return
+    raise ValueError(
+        f"Object path module {module_name!r} is outside trusted import prefixes. "
+        f"Set {TRUSTED_IMPORT_PREFIXES_ENV} to allow custom deployment config modules."
+    )
+
+
+def _module_matches_prefix(module_name: str, prefix: str) -> bool:
+    cleaned = prefix.strip()
+    if not cleaned:
+        return False
+    if cleaned.endswith((".", "_")):
+        return module_name.startswith(cleaned)
+    return module_name == cleaned or module_name.startswith(f"{cleaned}.")
+
+
 def load_server_config(path: str | None = None) -> GovoplanServerConfig:
     config_path = path or os.getenv("GOVOPLAN_SERVER_CONFIG") or "govoplan_core.server.default_config:get_server_config"
     try:

src/govoplan_core/server/registry.py

@@ -6,6 +6,7 @@ import importlib
 from govoplan_core.core.discovery import discover_module_manifests
 from govoplan_core.core.modules import ModuleManifest
 from govoplan_core.core.registry import PlatformRegistry, RegistryError
+from govoplan_core.server.config import validate_object_path
 
 ManifestFactory = Callable[[], ModuleManifest]
 
@@ -74,8 +75,10 @@ def build_platform_registry(enabled_modules: str | Iterable[str], *, manifest_fa
 
 def _load_builtin_manifest(module_id: str) -> ModuleManifest:
     target = _BUILTIN_MANIFESTS[module_id]
-    module_name, function_name = target.split(":", 1)
+    module_name, function_name = validate_object_path(target)
-    module = importlib.import_module(module_name)
+    if module_name != _BUILTIN_MANIFESTS[module_id].split(":", 1)[0]:
+        raise RegistryError(f"Built-in module manifest target changed unexpectedly: {module_id}")
+    module = importlib.import_module(module_name)  # nosemgrep: python.lang.security.audit.non-literal-import.non-literal-import
     factory = getattr(module, function_name)
     manifest = factory()
     if not isinstance(manifest, ModuleManifest):

tests/test_import_trust.py

@@ -0,0 +1,50 @@
+from __future__ import annotations
+
+import os
+import sys
+import tempfile
+import unittest
+from pathlib import Path
+from unittest.mock import patch
+
+from govoplan_core.server.config import import_object, validate_object_path
+from govoplan_core.server.registry import available_module_manifests
+
+
+class ImportTrustBoundaryTests(unittest.TestCase):
+    def test_object_path_validation_rejects_invalid_shapes(self) -> None:
+        with self.assertRaisesRegex(ValueError, "module:attribute"):
+            validate_object_path("govoplan_core.server.default_config")
+        with self.assertRaisesRegex(ValueError, "valid Python module"):
+            validate_object_path("os;system:path")
+        with self.assertRaisesRegex(ValueError, "public attribute"):
+            validate_object_path("govoplan_core.server.default_config:_private")
+
+    def test_import_object_allows_default_govoplan_config_path(self) -> None:
+        value = import_object("govoplan_core.server.default_config:get_server_config")
+        self.assertTrue(callable(value))
+
+    def test_import_object_rejects_untrusted_stdlib_module(self) -> None:
+        with self.assertRaisesRegex(ValueError, "outside trusted import prefixes"):
+            import_object("os:path")
+
+    def test_import_object_allows_explicit_custom_prefix(self) -> None:
+        with tempfile.TemporaryDirectory(prefix="govoplan-config-import-") as directory:
+            root = Path(directory)
+            (root / "custom_config.py").write_text("value = 42\n", encoding="utf-8")
+            sys.path.insert(0, str(root))
+            try:
+                with patch.dict(os.environ, {"GOVOPLAN_TRUSTED_IMPORT_PREFIXES": "custom_config"}):
+                    self.assertEqual(import_object("custom_config:value"), 42)
+            finally:
+                sys.path.remove(str(root))
+                sys.modules.pop("custom_config", None)
+
+    def test_builtin_manifest_imports_are_loaded_from_fixed_targets(self) -> None:
+        manifests = available_module_manifests(enabled_modules=("access",), ignore_load_errors=True)
+        if "access" in manifests:
+            self.assertEqual(manifests["access"].id, "access")
+
+
+if __name__ == "__main__":
+    unittest.main()

webui/src/platform/modules.ts

@@ -138,7 +138,7 @@ alreadyLoaded: PlatformWebModule[] = resolveInstalledWebModules(platformModules)
     let promise = remoteModuleCache.get(cacheKey);
     if (!promise) {
       promise = loadRemoteWebModule(module).catch((error) => {
-        console.warn(`GovOPlaN remote module ${module.id} was not loaded:`, error);
+        console.warn("GovOPlaN remote module was not loaded:", module.id, error);
         return null;
       });
       remoteModuleCache.set(cacheKey, promise);