feat(plugins): installs fetch only what the catalog names, verified (#5032)

#5032 "Plugin install takes its URL and checksum from the catalog, requires
the checksum, and extracts only inside the plugin folder". The install form
carried the download URL and checksum as hidden fields, so any admin POST
could make Steward fetch and import code from anywhere, unchecked, and a zip
member like ../x.py was written wherever it pointed.

- The route takes only the plugin name and reads download_url and
  checksum_sha256 from the configured catalogs; a name no repository lists
  is refused. Installs are audited (plugin.install / plugin.install_failed).
- The checksum is required; the name must be a plain identifier.
- extract_plugin refuses any member that resolves outside the plugin
  folder, caps the download at 50 MB and the unpacked size at 200 MB.
- The zip unpacks into a staging folder and is swapped in whole: a refused
  or failed install leaves the installed copy untouched, and an update no
  longer leaves the old version's deleted files behind.
- The catalog shows "Can't install: the catalog lists no checksum" in place
  of the button for an entry without one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

steward/core/plugin_manager.py

@@ -3,8 +3,8 @@ from __future__ import annotations
 import hashlib
 import logging
 import os
+import shutil
 import sys
-import tempfile
 import zipfile
 from pathlib import Path
 from typing import TYPE_CHECKING
@@ -290,19 +290,108 @@ def load_plugins(app: "Quart") -> None:
         logger.info("Plugin %r loaded (v%s)", name, meta.get("version", "?"))
 
 
+# Bounds on what an install may bring in: the download itself, and what it
+# unpacks to (a small zip can expand to fill the disk).
+MAX_PLUGIN_DOWNLOAD_BYTES = 50 * 1024 * 1024
+MAX_PLUGIN_UNPACKED_BYTES = 200 * 1024 * 1024
+
+
+class PluginInstallError(Exception):
+    """An install refused or failed; the message is shown to the admin."""
+
+
+def check_plugin_name(name: str) -> None:
+    """A plugin is imported by its folder name, so the name must be a plain
+    Python identifier — which also keeps it from naming a path."""
+    if not (name.isascii() and name.isidentifier()):
+        raise PluginInstallError(f"{name!r} is not a valid plugin name")
+
+
+def verify_checksum(content: bytes, expected_sha256: str) -> None:
+    """Refuse a download whose sha256 is missing from the catalog or differs."""
+    if not expected_sha256:
+        raise PluginInstallError(
+            "The catalog lists no checksum for this plugin, so Steward cannot "
+            "tell it is the file the catalog meant. Not installed.")
+    actual = hashlib.sha256(content).hexdigest()
+    if actual != expected_sha256.strip().lower():
+        raise PluginInstallError(f"Checksum mismatch: expected {expected_sha256}, got {actual}")
+
+
+def extract_plugin(content: bytes, name: str, dest: Path) -> None:
+    """Unpack a plugin zip into `dest`, which must not exist yet.
+
+    One top-level folder whose name starts with the plugin name is stripped:
+        name/                         (clean plugin zip)
+        name-v1.0.0/                  (release tag archive)
+    otherwise the zip is unpacked as it stands. Every member must land inside
+    `dest` — an absolute path or a `..` that climbs out refuses the whole zip —
+    and the unpacked total is capped.
+    """
+    import io
+    root = dest.resolve()
+    with zipfile.ZipFile(io.BytesIO(content)) as zf:
+        infos = zf.infolist()
+        if sum(i.file_size for i in infos) > MAX_PLUGIN_UNPACKED_BYTES:
+            raise PluginInstallError(
+                f"Unpacks to more than {MAX_PLUGIN_UNPACKED_BYTES // (1024 * 1024)} MB")
+        top_level = {i.filename.split("/")[0] for i in infos}
+        prefix = ""
+        if len(top_level) == 1:
+            candidate = next(iter(top_level))
+            if candidate.lower().startswith(name.lower()) and any(
+                    i.filename.startswith(candidate + "/") for i in infos):
+                prefix = candidate + "/"
+        planned: list[tuple[zipfile.ZipInfo, Path]] = []
+        for info in infos:
+            if prefix and not info.filename.startswith(prefix):
+                continue
+            rel = info.filename[len(prefix):]
+            if not rel:
+                continue
+            target = (root / rel).resolve()
+            if not target.is_relative_to(root) or target == root:
+                raise PluginInstallError(f"Refused: {info.filename!r} points outside the plugin folder")
+            planned.append((info, target))
+        root.mkdir(parents=True)
+        for info, target in planned:
+            if info.is_dir():
+                target.mkdir(parents=True, exist_ok=True)
+                continue
+            target.parent.mkdir(parents=True, exist_ok=True)
+            with zf.open(info) as src, open(target, "wb") as out:
+                shutil.copyfileobj(src, out)
+
+
+async def _download(url: str) -> bytes:
+    import httpx
+    chunks: list[bytes] = []
+    size = 0
+    async with httpx.AsyncClient(timeout=60, follow_redirects=True) as client:
+        async with client.stream("GET", url) as resp:
+            resp.raise_for_status()
+            async for chunk in resp.aiter_bytes():
+                size += len(chunk)
+                if size > MAX_PLUGIN_DOWNLOAD_BYTES:
+                    raise PluginInstallError(
+                        f"Download is larger than {MAX_PLUGIN_DOWNLOAD_BYTES // (1024 * 1024)} MB")
+                chunks.append(chunk)
+    return b"".join(chunks)
+
+
 async def download_and_install_plugin(
     app: "Quart",
     name: str,
     download_url: str,
     expected_sha256: str,
 ) -> tuple[bool, str]:
-    """Download a plugin zip, verify its checksum, and extract it to PLUGIN_DIR.
+    """Download a plugin zip named by the catalog, verify its checksum, and
+    unpack it into the install dir, replacing any earlier copy whole.
 
-    Returns (success, message). Does NOT hot-reload — call hot_reload_plugin()
-    after this to activate the plugin without a restart (new plugins only).
+    The URL and checksum must come from the catalog entry, never from the
+    request. Returns (success, message). Does NOT hot-reload — call
+    hot_reload_plugin() after this to activate a new plugin without a restart.
     """
-    import httpx
-
     # Downloads always land in the external (writable, persistent) install dir,
     # never the read-only bundled dir.
     plugin_dir = Path(app.config["PLUGIN_INSTALL_DIR"]).resolve()
@@ -314,68 +403,46 @@ async def download_and_install_plugin(
         sys.path.insert(0, plugin_dir_str)
 
     try:
+        check_plugin_name(name)
+        if not download_url:
+            raise PluginInstallError("The catalog lists no download URL for this plugin")
         logger.info("Downloading plugin %r from %s", name, download_url)
-        async with httpx.AsyncClient(timeout=60, follow_redirects=True) as client:
-            resp = await client.get(download_url)
-            resp.raise_for_status()
-        content = resp.content
-    except Exception as exc:
-        msg = f"Download failed: {exc}"
-        logger.error("Plugin %r: %s", name, msg)
-        return False, msg
+        try:
+            content = await _download(download_url)
+        except PluginInstallError:
+            raise
+        except Exception as exc:
+            raise PluginInstallError(f"Download failed: {exc}") from exc
+        verify_checksum(content, expected_sha256)
 
-    # Verify checksum if provided
-    if expected_sha256:
-        actual = hashlib.sha256(content).hexdigest()
-        if actual != expected_sha256.lower():
-            msg = f"Checksum mismatch: expected {expected_sha256}, got {actual}"
-            logger.error("Plugin %r: %s", name, msg)
-            return False, msg
-
-    # Extract the zip
-    try:
-        with tempfile.TemporaryDirectory() as tmpdir:
-            zip_path = Path(tmpdir) / f"{name}.zip"
-            zip_path.write_bytes(content)
-            with zipfile.ZipFile(zip_path) as zf:
-                members = zf.namelist()
-                # Collect unique top-level names (dirs and loose files)
-                top_level = {m.split("/")[0] for m in members}
-
-                dest = plugin_dir / name
-                dest.mkdir(parents=True, exist_ok=True)
-
-                # Detect a single top-level directory to strip. Handles:
-                #   name/               (clean plugin zip)
-                #   name-v1.0.0/        (GitHub release tag archive)
-                #   steward-plugins-main/name/  (whole-repo archive)
-                # Strategy: if there is exactly one top-level item and it is a
-                # directory whose name starts with the plugin name (case-insensitive),
-                # strip it. Otherwise extract flat into dest.
-                prefix: str | None = None
-                if len(top_level) == 1:
-                    candidate = list(top_level)[0]
-                    if candidate.lower().startswith(name.lower()):
-                        prefix = candidate + "/"
-
-                for member in members:
-                    if prefix:
-                        if not member.startswith(prefix):
-                            continue
-                        rel = member[len(prefix):]
-                    else:
-                        rel = member
-                    if not rel or rel.endswith("/"):
-                        if rel:
-                            (dest / rel.rstrip("/")).mkdir(parents=True, exist_ok=True)
-                        continue
-                    target = dest / rel
-                    target.parent.mkdir(parents=True, exist_ok=True)
-                    target.write_bytes(zf.read(member))
-    except Exception as exc:
-        msg = f"Extraction failed: {exc}"
-        logger.error("Plugin %r: %s", name, msg)
-        return False, msg
+        # Unpack beside the live copy, then swap: a failed or refused zip
+        # leaves the installed plugin untouched, and an update drops files the
+        # new version no longer ships.
+        plugin_dir.mkdir(parents=True, exist_ok=True)
+        dest = plugin_dir / name
+        staging = plugin_dir / f".{name}.installing"
+        old = plugin_dir / f".{name}.replaced"
+        for leftover in (staging, old):
+            shutil.rmtree(leftover, ignore_errors=True)
+        try:
+            extract_plugin(content, name, staging)
+        except PluginInstallError:
+            raise
+        except Exception as exc:
+            raise PluginInstallError(f"Extraction failed: {exc}") from exc
+        try:
+            if dest.exists():
+                dest.rename(old)
+            staging.rename(dest)
+        except OSError as exc:
+            if old.exists() and not dest.exists():
+                old.rename(dest)  # put the previous version back
+            raise PluginInstallError(f"Could not move the plugin into place: {exc}") from exc
+        shutil.rmtree(old, ignore_errors=True)
+    except PluginInstallError as exc:
+        shutil.rmtree(plugin_dir / f".{name}.installing", ignore_errors=True)
+        logger.error("Plugin %r: %s", name, exc)
+        return False, str(exc)
 
     # Run migrations for the newly installed plugin.
     # Include all plugin migration dirs so Alembic can resolve any previously-stamped

steward/settings/routes.py

@@ -820,23 +820,34 @@ async def plugins_catalog():
 @settings_bp.post("/plugins/install/<name>")
 @require_role(UserRole.admin)
 async def install_plugin(name: str):
-    """Download and install a plugin from the catalog, then hot-reload it."""
-    form = await request.form
-    download_url = form.get("download_url", "").strip()
-    checksum = form.get("checksum_sha256", "").strip()
+    """Download and install a plugin from the catalog, then hot-reload it.
 
-    if not download_url:
+    Only the plugin's name comes from the request. The download URL and
+    checksum are read from the configured catalogs, so a request cannot point
+    Steward at a file of its own choosing.
+    """
+    from steward.core.plugin_index import fetch_catalog
+    from steward.core.plugin_manager import download_and_install_plugin, hot_reload_plugin
+
+    async with current_app.db_sessionmaker() as db:
+        settings = await get_all_settings(db)
+    repo_urls = [r["url"] for r in _get_plugin_repos(settings) if r.get("url")]
+    entry = next((p for p in await fetch_catalog(repo_urls) if p.name == name), None)
+    if entry is None:
         return await render_template(
             "settings/_install_result.html",
             name=name, success=False,
-            message="No download URL provided.",
+            message="No configured plugin repository lists this plugin.",
         )
 
-    from steward.core.plugin_manager import download_and_install_plugin, hot_reload_plugin
-
     ok, msg = await download_and_install_plugin(
-        current_app._get_current_object(), name, download_url, checksum
+        current_app._get_current_object(), entry.name, entry.download_url, entry.checksum_sha256
     )
+    await log_audit(
+        current_app, session.get("user_id"), session.get("username", ""),
+        "plugin.install" if ok else "plugin.install_failed", entity_type="plugin",
+        entity_id=name, detail={"version": entry.version, "url": entry.download_url,
+                                "message": msg})
     if not ok:
         return await render_template(
             "settings/_install_result.html",

steward/templates/settings/_catalog_partial.html

@@ -56,7 +56,14 @@
              class="btn btn-ghost btn-sm" style="font-size:0.78rem;">Source</a>
           {% endif %}
 
-          {% if update_available %}
+          {% if (update_available or not is_installed) and not plugin.checksum_sha256 %}
+          {# Steward installs only what it can verify against the catalog's checksum #}
+          <span style="font-size:0.75rem;color:var(--text-muted);max-width:14rem;text-align:right;"
+                title="Add checksum_sha256 to this plugin's entry in the repository's index.yaml">
+            Can't {{ "update" if update_available else "install" }}: the catalog lists no checksum
+          </span>
+
+          {% elif update_available %}
           {# Installed but outdated — offer update (download + restart) #}
           <form method="post"
                 action="/settings/plugins/install/{{ plugin.name }}"
@@ -64,8 +71,6 @@
                 hx-target="#catalog-result-{{ plugin.name }}"
                 hx-swap="outerHTML"
                 style="margin:0;">{{ csrf_field() }}
-            <input type="hidden" name="download_url" value="{{ plugin.download_url }}">
-            <input type="hidden" name="checksum_sha256" value="{{ plugin.checksum_sha256 }}">
             <button type="submit" class="btn btn-warn btn-sm" style="font-size:0.78rem;">Update</button>
           </form>
 
@@ -91,8 +96,6 @@
                 hx-target="#catalog-result-{{ plugin.name }}"
                 hx-swap="outerHTML"
                 style="margin:0;">{{ csrf_field() }}
-            <input type="hidden" name="download_url" value="{{ plugin.download_url }}">
-            <input type="hidden" name="checksum_sha256" value="{{ plugin.checksum_sha256 }}">
             <button type="submit" class="btn btn-sm" style="font-size:0.78rem;">Install</button>
           </form>
           {% endif %}

tests/core/test_plugin_install.py

@@ -0,0 +1,116 @@
+"""Plugin installs fetch only what the catalog names, verify it, and unpack it
+only inside the plugin's own folder."""
+import hashlib
+import io
+import zipfile
+from types import SimpleNamespace
+from unittest.mock import AsyncMock, patch
+
+import pytest
+
+from steward.core import plugin_manager as pm
+
+
+def make_zip(files: dict[str, str]) -> bytes:
+    buf = io.BytesIO()
+    with zipfile.ZipFile(buf, "w") as zf:
+        for name, text in files.items():
+            zf.writestr(name, text)
+    return buf.getvalue()
+
+
+def sha(content: bytes) -> str:
+    return hashlib.sha256(content).hexdigest()
+
+
+@pytest.mark.parametrize("name", ["..", "a/b", "evil-name", "", "x.y", "ümlaut"])
+def test_name_must_be_an_identifier(name):
+    with pytest.raises(pm.PluginInstallError):
+        pm.check_plugin_name(name)
+
+
+def test_checksum_is_required_and_compared():
+    content = b"zip"
+    with pytest.raises(pm.PluginInstallError, match="no checksum"):
+        pm.verify_checksum(content, "")
+    with pytest.raises(pm.PluginInstallError, match="mismatch"):
+        pm.verify_checksum(content, "0" * 64)
+    pm.verify_checksum(content, sha(content).upper())
+
+
+def test_extract_strips_the_plugin_folder(tmp_path):
+    content = make_zip({"demo-v1.2/__init__.py": "x = 1", "demo-v1.2/sub/a.txt": "a"})
+    pm.extract_plugin(content, "demo", tmp_path / "demo")
+    assert (tmp_path / "demo" / "__init__.py").read_text() == "x = 1"
+    assert (tmp_path / "demo" / "sub" / "a.txt").read_text() == "a"
+
+
+@pytest.mark.parametrize("member", ["../escape.py", "demo/../../escape.py", "/abs.py"])
+def test_extract_refuses_members_outside_the_folder(tmp_path, member):
+    content = make_zip({"__init__.py": "", member: "boom"})
+    with pytest.raises(pm.PluginInstallError, match="outside"):
+        pm.extract_plugin(content, "demo", tmp_path / "plugins" / "demo")
+    assert not (tmp_path / "escape.py").exists()
+    assert not (tmp_path / "plugins" / "escape.py").exists()
+    assert not (tmp_path / "plugins" / "demo").exists()  # nothing half-written
+
+
+def test_extract_refuses_an_oversized_unpack(tmp_path, monkeypatch):
+    monkeypatch.setattr(pm, "MAX_PLUGIN_UNPACKED_BYTES", 10)
+    with pytest.raises(pm.PluginInstallError, match="Unpacks to more than"):
+        pm.extract_plugin(make_zip({"__init__.py": "x" * 11}), "demo", tmp_path / "demo")
+
+
+def _app(tmp_path):
+    return SimpleNamespace(config={"PLUGIN_INSTALL_DIR": str(tmp_path), "PLUGIN_DIRS": [str(tmp_path)],
+                                   "DATABASE_URL": ""})
+
+
+@pytest.mark.asyncio
+async def test_update_replaces_the_folder_whole(tmp_path):
+    old = tmp_path / "demo"
+    old.mkdir()
+    (old / "gone.py").write_text("old")
+    content = make_zip({"demo/__init__.py": "new"})
+    with patch.object(pm, "_download", new=AsyncMock(return_value=content)):
+        ok, msg = await pm.download_and_install_plugin(_app(tmp_path), "demo", "https://x/demo.zip", sha(content))
+    assert ok, msg
+    assert (old / "__init__.py").read_text() == "new"
+    assert not (old / "gone.py").exists()
+    assert sorted(p.name for p in tmp_path.iterdir()) == ["demo"]
+
+
+@pytest.mark.asyncio
+async def test_a_refused_download_leaves_the_installed_copy(tmp_path):
+    old = tmp_path / "demo"
+    old.mkdir()
+    (old / "__init__.py").write_text("old")
+    content = make_zip({"demo/__init__.py": "new"})
+    with patch.object(pm, "_download", new=AsyncMock(return_value=content)):
+        ok, msg = await pm.download_and_install_plugin(_app(tmp_path), "demo", "https://x/demo.zip", "")
+    assert not ok and "no checksum" in msg
+    assert (old / "__init__.py").read_text() == "old"
+    assert sorted(p.name for p in tmp_path.iterdir()) == ["demo"]
+
+
+@pytest.mark.asyncio
+async def test_route_takes_url_and_checksum_from_the_catalog(client):
+    async with client.session_transaction() as sess:
+        sess.update(user_id="u", user_role="admin", username="admin", _csrf="t")
+    entry = SimpleNamespace(name="demo", version="1.0", download_url="https://catalog/demo.zip",
+                            checksum_sha256="abc")
+    install = AsyncMock(return_value=(False, "stopped here"))
+    with patch("steward.settings.routes.get_all_settings", new=AsyncMock(return_value={})), \
+         patch("steward.settings.routes._get_plugin_repos", return_value=[{"url": "https://catalog/"}]), \
+         patch("steward.core.plugin_index.fetch_catalog", new=AsyncMock(return_value=[entry])), \
+         patch("steward.core.plugin_manager.download_and_install_plugin", new=install), \
+         patch("steward.settings.routes.log_audit", new=AsyncMock()):
+        resp = await client.post("/settings/plugins/install/demo", headers={"X-CSRF-Token": "t"}, form={
+            "download_url": "https://attacker/evil.zip", "checksum_sha256": "dead"})
+        assert resp.status_code == 200
+        assert install.await_args.args[1:] == ("demo", "https://catalog/demo.zip", "abc")
+
+        install.reset_mock()
+        resp = await client.post("/settings/plugins/install/other", headers={"X-CSRF-Token": "t"})
+        assert "No configured plugin repository lists this plugin" in await resp.get_data(as_text=True)
+        install.assert_not_awaited()