agnes-the-ai-analyst/app/api/scripts.py
ZdenekSrotyr 24e81fb671
fix(security): gate Script-API /run on admin role (#44) (#92)
* fix(security): gate Script-API /run on admin role (#44)

The AST + string-blocklist sandbox in `_execute_script` is defense-in-depth,
not a primary trust boundary. It does not block `vars()`, `type()`, or
`__class__.__bases__` introspection chains, and the string blocklist is
trivially evadable via concatenation/dunder encoding. Treat the role gate
as the actual barrier: only admin can run scripts.

- `POST /api/scripts/run` and `POST /api/scripts/{id}/run` now require admin.
- `POST /api/scripts/deploy` stays analyst-accessible (storing != executing).
- Existing /run tests retargeted to admin_token; added regression tests
  asserting analyst → 403 on both endpoints.
- CHANGELOG: BREAKING (security) bullet under Unreleased/Changed.

Closes #44.

* fix(security): admin-gate /deploy + harden sandbox blocklist (review #92)

Reviewer of PR #92 flagged three MUST-FIXes that #44 wasn't fully closed:

1. /api/scripts/deploy still accepted analyst → planted-script attack
   path (analyst plants malicious source, waits for admin to /run).
   Now: /deploy also requires admin; the entire Script API is admin-only.

2. The "Minimum (same-day)" blocklist mitigations from issue #44 weren't
   applied. Added the introspection-chain dunders that the issue PoC
   pivots through: __subclasses__, __globals__, __class__, __base__,
   __bases__, __mro__, __dict__, __code__, __builtins__. Plus `vars`
   in BLOCKED_FUNCTIONS. Deliberately NOT adding __init__ /
   __getattribute__ (substring match would flag every legit `def __init__`)
   nor `type`/`dir` (frequent in legitimate admin scripts). Documented
   the trade-off inline.

3. Tests didn't cover the actual PoC payload nor non-analyst non-admin
   roles. Added test_run_pwn_payload_blocked parametrized over the issue's
   own PoC + two equivalent variants (lambda+__globals__, __mro__
   traversal); these stay green only as long as the dunder list does.
   test_*_requires_admin tests now parametrize over (analyst, viewer,
   km_admin) so all three non-admin core roles are pinned at 403.

Conftest extension: seeded_app now exposes viewer_token and
km_admin_token as siblings to admin_token / analyst_token.

CHANGELOG bullet updated to reflect /deploy gate change and new
internal regression tests. 35/35 scripts tests pass locally.

Refs review of #92.

* fix(tests): test_security TestScriptSandbox needs admin token after #44 hardening

CI failure on PR #92 caught a missed test file. tests/test_security.py
seeded only an analyst user and used the analyst token to drive sandbox
tests. After the #44 admin-gate (deploy + run both admin-only), every
sandbox test got 403 from the role gate before the AST/string check
could run, so 'blocks os.system' / 'blocks eval' / etc. all failed.

Fix: extend the fixture to also seed an admin user and return the admin
token. Sandbox tests now reach the sandbox layer; access-control tests
further down in the module continue to use the analyst that was kept
around. 41/41 test_security.py tests pass locally.

* fix(security): #92 round-3 — gate GET /api/scripts on admin role

Devin Review caught: GET /api/scripts (app/api/scripts.py:44-51) was
left on Depends(get_current_user) when the rest of the API moved to
admin-only. ScriptRepository.list_all() does SELECT * FROM script_registry
which returns ALL columns including 'source' (the full script body).
So any authenticated user (viewer / analyst / km_admin) could read
admin-deployed scripts — leak of code that may contain credentials,
business logic, or admin-only operational details.

CHANGELOG already says 'The entire Script API is now admin-only',
which was true for /deploy, /run, /{id}/run, DELETE — just not for
GET. Now consistent: every Script endpoint requires admin.

Tests:
- New parametrized test_list_scripts_requires_admin over (analyst,
  viewer, km_admin) tokens — all assert 403.
- Updated test_list_scripts_empty in both test_scripts_api.py and
  test_api_scripts.py to use admin_token.

79 tests pass.

Refs Devin Review of #92.

* fix: cleanup unused imports, stale docstrings, and incomplete CHANGELOG

- Remove unused imports: Path, List, get_current_user (ruff F401)
- Trim docstrings to describe current behavior, not change history
- CHANGELOG now lists GET /api/scripts among admin-gated endpoints
- Remove diff-commenting inline comments from tests

Co-Authored-By: zdenek.srotyr <zdenek.srotyr@keboola.com>

* fix: merge duplicate Changed sections into one per CLAUDE.md convention

Co-Authored-By: zdenek.srotyr <zdenek.srotyr@keboola.com>

---------

Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
2026-04-27 21:13:56 +02:00

237 lines
7.6 KiB
Python

"""Script management and execution endpoints."""
import os
import subprocess
import sys
import tempfile
import uuid
from fastapi import APIRouter, Depends, HTTPException
from pydantic import BaseModel
from typing import Optional
import duckdb
from app.auth.dependencies import require_role, _get_db
from src.rbac import Role
from src.repositories.notifications import ScriptRepository
router = APIRouter(prefix="/api/scripts", tags=["scripts"])
SCRIPT_TIMEOUT = int(os.environ.get("SCRIPT_TIMEOUT", "300")) # 5 min default
SCRIPT_MAX_OUTPUT = int(os.environ.get("SCRIPT_MAX_OUTPUT", "65536")) # 64KB
class DeployScriptRequest(BaseModel):
name: str
source: str
schedule: Optional[str] = None
class RunScriptRequest(BaseModel):
name: Optional[str] = None
source: Optional[str] = None
class ScriptResponse(BaseModel):
id: str
name: str
schedule: Optional[str]
owner: Optional[str]
@router.get("")
async def list_scripts(
user: dict = Depends(require_role(Role.ADMIN)),
conn: duckdb.DuckDBPyConnection = Depends(_get_db),
):
"""List deployed scripts. Admin-only."""
repo = ScriptRepository(conn)
scripts = repo.list_all()
return {"scripts": scripts, "count": len(scripts)}
@router.post("/deploy", status_code=201)
async def deploy_script(
request: DeployScriptRequest,
user: dict = Depends(require_role(Role.ADMIN)),
conn: duckdb.DuckDBPyConnection = Depends(_get_db),
):
"""Deploy a Python script to be run on the server (optionally on schedule). Admin-only."""
repo = ScriptRepository(conn)
script_id = str(uuid.uuid4())
repo.deploy(
id=script_id,
name=request.name,
owner=user["id"],
schedule=request.schedule,
source=request.source,
)
return ScriptResponse(
id=script_id, name=request.name,
schedule=request.schedule, owner=user["id"],
)
@router.post("/{script_id}/run")
async def run_deployed_script(
script_id: str,
user: dict = Depends(require_role(Role.ADMIN)),
conn: duckdb.DuckDBPyConnection = Depends(_get_db),
):
"""Run a deployed script by ID. Admin-only."""
repo = ScriptRepository(conn)
script = repo.get(script_id)
if not script:
raise HTTPException(status_code=404, detail="Script not found")
return _execute_script(script["source"], script["name"])
@router.post("/run")
async def run_adhoc_script(
request: RunScriptRequest,
user: dict = Depends(require_role(Role.ADMIN)),
):
"""Run an ad-hoc Python script (not deployed). Admin-only."""
if not request.source:
raise HTTPException(status_code=400, detail="Script source required")
return _execute_script(request.source, request.name or "adhoc")
@router.delete("/{script_id}", status_code=204)
async def undeploy_script(
script_id: str,
user: dict = Depends(require_role(Role.ADMIN)),
conn: duckdb.DuckDBPyConnection = Depends(_get_db),
):
repo = ScriptRepository(conn)
if not repo.get(script_id):
raise HTTPException(status_code=404, detail="Script not found")
repo.undeploy(script_id)
def _execute_script(source: str, name: str) -> dict:
"""Execute a Python script in a sandboxed subprocess.
The blocklist below is defense-in-depth, not a primary trust boundary.
The role gate on the route (admin-only) is the actual boundary; the
blocklist catches obvious mistakes, not a hostile admin."""
# Comprehensive safety checks — block dangerous patterns
blocked_patterns = [
# Direct imports of dangerous modules
"import subprocess", "from subprocess",
"import shutil", "from shutil",
"import ctypes", "from ctypes",
"import importlib", "from importlib",
"import socket", "from socket",
"import requests", "from requests",
"import httpx", "from httpx",
"import urllib", "from urllib",
"import http", "from http",
# Dynamic import bypasses
"__import__",
"importlib",
# Code execution bypasses
"exec(",
"eval(",
"compile(",
# OS-level access
"import os", "from os",
"import sys", "from sys",
"import signal", "from signal",
# File access bypasses
"open(",
"pathlib",
# Dangerous builtins
"globals()",
"locals()",
"getattr(",
"setattr(",
"delattr(",
"breakpoint(",
# Introspection-chain dunders that can pivot to RCE.
# `__init__`/`__getattribute__` deliberately omitted: substring
# match would flag every `def __init__(self):`.
"__subclasses__",
"__globals__",
"__class__",
"__base__",
"__bases__",
"__mro__",
"__dict__",
"__code__",
"__builtins__",
]
import ast
BLOCKED_MODULES = {"os", "sys", "subprocess", "shutil", "ctypes", "importlib", "socket",
"requests", "httpx", "urllib", "http", "signal", "pathlib", "builtins"}
BLOCKED_FUNCTIONS = {"exec", "eval", "compile", "open", "globals", "locals",
"getattr", "setattr", "delattr", "breakpoint", "__import__",
"vars"}
try:
tree = ast.parse(source)
except SyntaxError as e:
raise HTTPException(status_code=400, detail=f"Script syntax error: {e}")
for node in ast.walk(tree):
if isinstance(node, ast.Import):
for alias in node.names:
if alias.name.split(".")[0] in BLOCKED_MODULES:
raise HTTPException(status_code=400, detail=f"Blocked import: {alias.name}")
elif isinstance(node, ast.ImportFrom):
if node.module and node.module.split(".")[0] in BLOCKED_MODULES:
raise HTTPException(status_code=400, detail=f"Blocked import: {node.module}")
elif isinstance(node, ast.Call):
if isinstance(node.func, ast.Name) and node.func.id in BLOCKED_FUNCTIONS:
raise HTTPException(status_code=400, detail=f"Blocked function: {node.func.id}")
source_lower = source.lower()
for pattern in blocked_patterns:
if pattern.lower() in source_lower:
raise HTTPException(
status_code=400,
detail=f"Script contains disallowed pattern: {pattern.split('(')[0].strip()}",
)
data_dir = os.environ.get("DATA_DIR", "./data")
with tempfile.NamedTemporaryFile(mode="w", suffix=".py", delete=False) as f:
f.write(source)
f.flush()
script_path = f.name
try:
result = subprocess.run(
[sys.executable, script_path],
capture_output=True,
text=True,
timeout=SCRIPT_TIMEOUT,
env={
"PATH": "/usr/bin:/usr/local/bin",
"DATA_DIR": data_dir,
"HOME": "/tmp",
# Deliberately exclude VIRTUAL_ENV and PYTHONPATH
# to prevent access to installed packages
},
cwd="/tmp", # restrict working directory
)
stdout = result.stdout[:SCRIPT_MAX_OUTPUT]
stderr = result.stderr[:SCRIPT_MAX_OUTPUT]
return {
"name": name,
"exit_code": result.returncode,
"stdout": stdout,
"stderr": stderr,
"truncated": len(result.stdout) > SCRIPT_MAX_OUTPUT or len(result.stderr) > SCRIPT_MAX_OUTPUT,
}
except subprocess.TimeoutExpired:
return {
"name": name,
"exit_code": -1,
"stdout": "",
"stderr": f"Script timed out after {SCRIPT_TIMEOUT}s",
"truncated": False,
}
finally:
os.unlink(script_path)