fix(tasks): gate cookbook serve task execution (#5235)

routes/task_routes.py

@@ -11,10 +11,14 @@ from fastapi import APIRouter, HTTPException, Request
 from pydantic import BaseModel
 
 from core.database import SessionLocal, ScheduledTask, TaskRun
-from core.middleware import INTERNAL_TOOL_USER
 from core.constants import internal_api_base
 from src.auth_helpers import get_current_user
 from src.constants import DATA_DIR, EMAIL_URGENCY_CACHE_DIR
+from src.task_action_policy import (
+    ADMIN_ONLY_TASK_ACTIONS,
+    is_admin_only_task_action,
+    owner_has_admin_task_privileges,
+)
 from src.task_scheduler import compute_next_run, HOUSEKEEPING_DEFAULTS
 from routes.prefs_routes import _load_for_user, _save_for_user
 
@@ -417,28 +421,18 @@ def setup_task_routes(task_scheduler) -> APIRouter:
                 db.close()
         return {"ok": True, "opened": True, "enabled": bool(prefs.get("tasks_enabled")), "resumed": resumed}
 
-    # Actions that execute shell/SSH commands — restricted to admins.
+    # Actions that execute shell/SSH commands or cross into admin-only
+    # Cookbook serving surfaces — restricted to admins.
     # Non-admin users cannot create tasks with these action types via the
     # API. See review CRIT-C.
-    _ADMIN_ONLY_ACTIONS = {"run_local", "run_script", "ssh_command"}
+    _ADMIN_ONLY_ACTIONS = ADMIN_ONLY_TASK_ACTIONS
 
     def _is_admin(user: str | None) -> bool:
-        if not user:
-            return False
-        # In-process tool-loopback marker — AuthMiddleware validated
-        # the internal token + loopback client before stamping this,
-        # so treat as admin-equivalent.
-        if user == INTERNAL_TOOL_USER:
-            return True
-        try:
-            from core.auth import AuthManager
-            auth = AuthManager()
-            if not auth.is_configured:
-                # Unconfigured single-user deploy: trust the local owner.
-                return True
-            return bool(auth.is_admin(user))
-        except Exception:
-            return False
+        return owner_has_admin_task_privileges(user)
+
+    def _require_admin_for_task_action(user: str | None, task_type: str | None, action: str | None) -> None:
+        if is_admin_only_task_action(task_type, action) and not _is_admin(user):
+            raise HTTPException(403, f"Action '{action}' requires admin privileges")
 
     def _validate_then_task_id(db, then_task_id: Optional[str], user: Optional[str], current_task_id: Optional[str] = None) -> Optional[str]:
         target_id = (then_task_id or "").strip()
@@ -466,8 +460,7 @@ def setup_task_routes(task_scheduler) -> APIRouter:
         # Block shell-executing action types for non-admins. action_run_local
         # uses subprocess.run(shell=True) and ssh_command / run_script run
         # arbitrary commands.
-        if req.task_type == "action" and req.action in _ADMIN_ONLY_ACTIONS and not _is_admin(user):
-            raise HTTPException(403, f"Action '{req.action}' requires admin privileges")
+        _require_admin_for_task_action(user, req.task_type, req.action)
         if req.trigger_type == "schedule" and not req.schedule:
             raise HTTPException(400, "Schedule is required for schedule-triggered tasks")
         if req.trigger_type == "schedule" and req.schedule == "cron" and not req.cron_expression:
@@ -681,6 +674,10 @@ def setup_task_routes(task_scheduler) -> APIRouter:
             if user and task.owner != user:
                 raise HTTPException(403, "Access denied")
 
+            next_task_type = req.task_type if req.task_type is not None else task.task_type
+            next_action = req.action if req.action is not None else task.action
+            _require_admin_for_task_action(user, next_task_type, next_action)
+
             if req.name is not None:
                 task.name = req.name
             if req.prompt is not None:
@@ -688,9 +685,6 @@ def setup_task_routes(task_scheduler) -> APIRouter:
             if req.task_type is not None:
                 task.task_type = req.task_type
             if req.action is not None:
-                # Same admin-only gate as create — see CRIT-C.
-                if req.action in _ADMIN_ONLY_ACTIONS and not _is_admin(user):
-                    raise HTTPException(403, f"Action '{req.action}' requires admin privileges")
                 task.action = req.action
             if req.output_target is not None:
                 task.output_target = req.output_target
@@ -807,6 +801,7 @@ def setup_task_routes(task_scheduler) -> APIRouter:
                 raise HTTPException(404, "Task not found")
             if user and task.owner != user:
                 raise HTTPException(403, "Access denied")
+            _require_admin_for_task_action(user, task.task_type, task.action)
             task.status = "active"
             if (task.trigger_type or "schedule") == "schedule":
                 task.next_run = compute_next_run(
@@ -869,6 +864,7 @@ def setup_task_routes(task_scheduler) -> APIRouter:
                 raise HTTPException(404, "Task not found")
             if user and task.owner != user:
                 raise HTTPException(403, "Access denied")
+            _require_admin_for_task_action(user, task.task_type, task.action)
         finally:
             db.close()
         started = await task_scheduler.run_task_now(task_id, force=force)
@@ -1058,6 +1054,14 @@ def setup_task_routes(task_scheduler) -> APIRouter:
             ).first()
             if not task:
                 raise HTTPException(404, "Not found")
+            if (
+                is_admin_only_task_action(task.task_type, task.action)
+                and not owner_has_admin_task_privileges(task.owner)
+            ):
+                task.status = "paused"
+                task.next_run = None
+                db.commit()
+                raise HTTPException(403, f"Action '{task.action}' requires admin privileges")
         finally:
             db.close()
         started = await task_scheduler.run_task_now(task_id)

src/task_action_policy.py

@@ -0,0 +1,47 @@
+"""Shared privilege policy for scheduled task actions."""
+
+from __future__ import annotations
+
+ADMIN_ONLY_TASK_ACTIONS = frozenset({
+    "run_local",
+    "run_script",
+    "ssh_command",
+    "cookbook_serve",
+})
+
+
+def is_admin_only_task_action(task_type: str | None, action: str | None) -> bool:
+    return (task_type or "llm") == "action" and (action or "") in ADMIN_ONLY_TASK_ACTIONS
+
+
+def owner_has_admin_task_privileges(owner: str | None) -> bool:
+    try:
+        from src.auth_helpers import _auth_disabled
+        if _auth_disabled():
+            return True
+    except Exception:
+        pass
+
+    if owner:
+        try:
+            from core.middleware import INTERNAL_TOOL_USER
+            if owner == INTERNAL_TOOL_USER:
+                return True
+        except Exception:
+            pass
+
+    try:
+        from core.auth import AuthManager
+        auth = AuthManager()
+        if not auth.is_configured:
+            return True
+        if not owner:
+            return False
+        return bool(auth.is_admin(owner))
+    except Exception:
+        pass
+
+    if not owner:
+        return False
+
+    return False

src/task_scheduler.py

@@ -10,6 +10,10 @@ from datetime import datetime, timedelta, timezone
 from typing import Any, Awaitable, Callable, Dict, Tuple
 
 from core.auth import RESERVED_USERNAMES
+from src.task_action_policy import (
+    is_admin_only_task_action,
+    owner_has_admin_task_privileges,
+)
 
 logger = logging.getLogger(__name__)
 
@@ -821,6 +825,28 @@ class TaskScheduler:
                     db.commit()
                 return
 
+            if (
+                is_admin_only_task_action(task.task_type, task.action)
+                and not owner_has_admin_task_privileges(task.owner)
+            ):
+                msg = f"Action '{task.action}' requires admin privileges"
+                blocked = db.query(TaskRun).filter(TaskRun.id == run_id).first()
+                if blocked:
+                    blocked.status = "error"
+                    blocked.result = msg
+                    blocked.error = msg
+                    blocked.finished_at = _utcnow()
+                task.status = "paused"
+                task.next_run = None
+                task.last_run = _utcnow()
+                logger.warning(
+                    "Paused admin-only task %s for non-admin owner %r",
+                    task_id,
+                    task.owner,
+                )
+                db.commit()
+                return
+
             if gate_foreground:
                 waiting = db.query(TaskRun).filter(TaskRun.id == run_id).first()
                 if waiting and waiting.status == "queued":

tests/test_auth_regressions.py

@@ -334,16 +334,11 @@ def test_pop_notifications_owner_filtered():
 def test_admin_only_actions_set_contains_shell_runners():
     """The constant defining shell-executing action types must include
     the three risky entries. Catches accidental removal."""
-    from routes import task_routes
-    # `_ADMIN_ONLY_ACTIONS` is a closure constant. Easiest pin: re-read
-    # the source and check for the three risky entries + the admin gate
-    # wording.
-    src = open(task_routes.__file__, encoding="utf-8").read()
-    assert '"run_local"' in src
-    assert '"run_script"' in src
-    assert '"ssh_command"' in src
-    # And the gate is wired into both create and update paths.
-    assert "Action '" in src and "requires admin privileges" in src
+    from src.task_action_policy import ADMIN_ONLY_TASK_ACTIONS
+
+    assert "run_local" in ADMIN_ONLY_TASK_ACTIONS
+    assert "run_script" in ADMIN_ONLY_TASK_ACTIONS
+    assert "ssh_command" in ADMIN_ONLY_TASK_ACTIONS
 
 
 def test_task_create_notification_default_allows_action_specific_defaults():

tests/test_task_cookbook_admin_gate.py

@@ -0,0 +1,350 @@
+"""Task CRUD must not let non-admins schedule Cookbook serve actions."""
+
+import sys
+from datetime import datetime, timedelta, timezone
+from types import SimpleNamespace
+from unittest.mock import MagicMock
+
+import pytest
+from fastapi import HTTPException
+from sqlalchemy import create_engine
+from sqlalchemy.orm import sessionmaker
+from sqlalchemy.pool import NullPool
+
+from tests.helpers.import_state import clear_fake_database_modules
+
+clear_fake_database_modules()
+
+import core.auth as core_auth
+import core.database as cdb
+import routes.task_routes as task_routes
+from core.database import ScheduledTask
+from core.database import TaskRun
+from src.task_scheduler import TaskScheduler
+
+_REAL_DATABASE_ATTRS = {
+    "Base": cdb.Base,
+    "SessionLocal": cdb.SessionLocal,
+    "ScheduledTask": ScheduledTask,
+    "TaskRun": TaskRun,
+}
+if hasattr(cdb, "engine"):
+    _REAL_DATABASE_ATTRS["engine"] = cdb.engine
+
+
+def _restore_module_binding(monkeypatch, name, module):
+    monkeypatch.setitem(sys.modules, name, module)
+    parent_name, _, attr = name.rpartition(".")
+    parent = sys.modules.get(parent_name)
+    if parent is not None:
+        monkeypatch.setattr(parent, attr, module, raising=False)
+
+
+@pytest.fixture()
+def task_db(monkeypatch, tmp_path):
+    _restore_module_binding(monkeypatch, "core.database", cdb)
+    for attr, value in _REAL_DATABASE_ATTRS.items():
+        monkeypatch.setattr(cdb, attr, value, raising=False)
+    engine = create_engine(
+        f"sqlite:///{tmp_path / 'tasks.db'}",
+        connect_args={"check_same_thread": False},
+        poolclass=NullPool,
+    )
+    cdb.Base.metadata.create_all(engine)
+    testing_session = sessionmaker(bind=engine, autoflush=False, autocommit=False)
+    monkeypatch.setattr(task_routes, "SessionLocal", testing_session)
+    monkeypatch.setattr(cdb, "SessionLocal", testing_session)
+    return testing_session
+
+
+@pytest.fixture()
+def configured_auth(monkeypatch):
+    _restore_module_binding(monkeypatch, "core.auth", core_auth)
+    monkeypatch.setenv("AUTH_ENABLED", "true")
+
+    class FakeAuthManager:
+        is_configured = True
+
+        def is_admin(self, user):
+            return user == "admin"
+
+    monkeypatch.setattr(core_auth, "AuthManager", FakeAuthManager)
+
+
+@pytest.fixture()
+def builtin_action_info(monkeypatch):
+    mod = sys.modules.get("src.builtin_actions")
+    if mod is None:
+        import src.builtin_actions as mod
+    monkeypatch.setattr(
+        mod,
+        "BUILTIN_ACTION_INFO",
+        {
+            "summarize_emails": "Summarize emails",
+            "cookbook_serve": "Serve Cookbook model",
+        },
+        raising=False,
+    )
+
+
+def _req(user):
+    return SimpleNamespace(state=SimpleNamespace(current_user=user))
+
+
+def _endpoint(method, path):
+    router = task_routes.setup_task_routes(MagicMock())
+    for route in router.routes:
+        if getattr(route, "path", None) == path and method in getattr(route, "methods", set()):
+            return route.endpoint
+    raise RuntimeError(f"{method} {path} not found")
+
+
+def _cookbook_create_req():
+    return task_routes.TaskCreate(
+        name="Serve test model",
+        prompt="{}",
+        task_type="action",
+        action="cookbook_serve",
+        trigger_type="webhook",
+    )
+
+
+def _seed_action_task(
+    session_factory,
+    task_id,
+    owner,
+    action="summarize_emails",
+    *,
+    task_type="action",
+    webhook_token=None,
+    next_run=None,
+):
+    db = session_factory()
+    try:
+        task = ScheduledTask(
+            id=task_id,
+            owner=owner,
+            name=task_id,
+            prompt="{}",
+            task_type=task_type,
+            action=action,
+            trigger_type="webhook",
+            status="active",
+            output_target="session",
+            webhook_token=webhook_token,
+            next_run=next_run,
+        )
+        db.add(task)
+        db.commit()
+    finally:
+        db.close()
+
+
+@pytest.mark.asyncio
+async def test_non_admin_cannot_create_cookbook_serve_task(task_db, configured_auth):
+    create_task = _endpoint("POST", "/api/tasks")
+
+    with pytest.raises(HTTPException) as exc:
+        await create_task(_req("alice"), _cookbook_create_req())
+
+    assert exc.value.status_code == 403
+    db = task_db()
+    try:
+        assert db.query(ScheduledTask).count() == 0
+    finally:
+        db.close()
+
+
+@pytest.mark.asyncio
+async def test_non_admin_cannot_update_task_to_cookbook_serve(task_db, configured_auth):
+    _seed_action_task(task_db, "alice-task", "alice")
+    update_task = _endpoint("PUT", "/api/tasks/{task_id}")
+
+    with pytest.raises(HTTPException) as exc:
+        await update_task(
+            _req("alice"),
+            "alice-task",
+            task_routes.TaskUpdate(action="cookbook_serve"),
+        )
+
+    assert exc.value.status_code == 403
+    db = task_db()
+    try:
+        task = db.query(ScheduledTask).filter(ScheduledTask.id == "alice-task").first()
+        assert task.action == "summarize_emails"
+    finally:
+        db.close()
+
+
+@pytest.mark.asyncio
+async def test_non_admin_cannot_update_task_type_to_activate_existing_cookbook_serve(
+    task_db, configured_auth
+):
+    _seed_action_task(
+        task_db,
+        "alice-task",
+        "alice",
+        action="cookbook_serve",
+        task_type="llm",
+    )
+    update_task = _endpoint("PUT", "/api/tasks/{task_id}")
+
+    with pytest.raises(HTTPException) as exc:
+        await update_task(
+            _req("alice"),
+            "alice-task",
+            task_routes.TaskUpdate(task_type="action"),
+        )
+
+    assert exc.value.status_code == 403
+    db = task_db()
+    try:
+        task = db.query(ScheduledTask).filter(ScheduledTask.id == "alice-task").first()
+        assert task.task_type == "llm"
+        assert task.action == "cookbook_serve"
+    finally:
+        db.close()
+
+
+@pytest.mark.asyncio
+async def test_non_admin_cannot_manually_run_existing_cookbook_serve_task(
+    task_db, configured_auth
+):
+    _seed_action_task(task_db, "alice-task", "alice", action="cookbook_serve")
+    scheduler = SimpleNamespace(run_task_now=MagicMock())
+    router = task_routes.setup_task_routes(scheduler)
+    for route in router.routes:
+        if getattr(route, "path", None) == "/api/tasks/{task_id}/run":
+            run_task = route.endpoint
+            break
+    else:
+        raise RuntimeError("POST /api/tasks/{task_id}/run not found")
+
+    with pytest.raises(HTTPException) as exc:
+        await run_task(_req("alice"), "alice-task")
+
+    assert exc.value.status_code == 403
+    scheduler.run_task_now.assert_not_called()
+
+
+@pytest.mark.asyncio
+async def test_webhook_rejects_stale_non_admin_cookbook_serve_task(
+    task_db, configured_auth
+):
+    _seed_action_task(
+        task_db,
+        "alice-task",
+        "alice",
+        action="cookbook_serve",
+        webhook_token="secret",
+    )
+    webhook_trigger = _endpoint("POST", "/api/tasks/{task_id}/webhook/{token}")
+
+    with pytest.raises(HTTPException) as exc:
+        await webhook_trigger("alice-task", "secret")
+
+    assert exc.value.status_code == 403
+    db = task_db()
+    try:
+        task = db.query(ScheduledTask).filter(ScheduledTask.id == "alice-task").first()
+        assert task.status == "paused"
+        assert task.next_run is None
+    finally:
+        db.close()
+
+
+@pytest.mark.asyncio
+async def test_scheduler_pauses_stale_non_admin_cookbook_serve_task(
+    task_db, configured_auth
+):
+    due = datetime.now(timezone.utc).replace(tzinfo=None) - timedelta(minutes=1)
+    _seed_action_task(
+        task_db,
+        "alice-task",
+        "alice",
+        action="cookbook_serve",
+        next_run=due,
+    )
+    db = task_db()
+    try:
+        db.add(TaskRun(id="run-1", task_id="alice-task", status="queued"))
+        db.commit()
+    finally:
+        db.close()
+
+    scheduler = TaskScheduler.__new__(TaskScheduler)
+    scheduler._task_handles = {}
+    await scheduler._execute_task_locked(
+        "alice-task",
+        "run-1",
+        gate_foreground=False,
+        release_executing=False,
+    )
+
+    db = task_db()
+    try:
+        task = db.query(ScheduledTask).filter(ScheduledTask.id == "alice-task").first()
+        run = db.query(TaskRun).filter(TaskRun.id == "run-1").first()
+        assert task.status == "paused"
+        assert task.next_run is None
+        assert run.status == "error"
+        assert run.error == "Action 'cookbook_serve' requires admin privileges"
+    finally:
+        db.close()
+
+
+@pytest.mark.asyncio
+async def test_non_admin_action_metadata_hides_cookbook_serve(
+    configured_auth, builtin_action_info
+):
+    list_actions = _endpoint("GET", "/api/tasks/meta/actions")
+
+    out = await list_actions(_req("alice"))
+
+    action_names = {action["name"] for action in out["actions"]}
+    assert "cookbook_serve" not in action_names
+
+
+@pytest.mark.asyncio
+async def test_admin_can_create_cookbook_serve_task(task_db, configured_auth):
+    create_task = _endpoint("POST", "/api/tasks")
+
+    out = await create_task(_req("admin"), _cookbook_create_req())
+
+    assert out["action"] == "cookbook_serve"
+    db = task_db()
+    try:
+        task = db.query(ScheduledTask).filter(ScheduledTask.id == out["id"]).first()
+        assert task.owner == "admin"
+    finally:
+        db.close()
+
+
+@pytest.mark.asyncio
+async def test_admin_action_metadata_includes_cookbook_serve(
+    configured_auth, builtin_action_info
+):
+    list_actions = _endpoint("GET", "/api/tasks/meta/actions")
+
+    out = await list_actions(_req("admin"))
+
+    action_names = {action["name"] for action in out["actions"]}
+    assert "cookbook_serve" in action_names
+
+
+@pytest.mark.asyncio
+async def test_auth_disabled_single_user_can_create_cookbook_serve_task(
+    monkeypatch, task_db
+):
+    monkeypatch.setenv("AUTH_ENABLED", "false")
+    create_task = _endpoint("POST", "/api/tasks")
+
+    out = await create_task(_req(None), _cookbook_create_req())
+
+    assert out["action"] == "cookbook_serve"
+    db = task_db()
+    try:
+        task = db.query(ScheduledTask).filter(ScheduledTask.id == out["id"]).first()
+        assert task.owner is None
+    finally:
+        db.close()