* 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>
237 lines
7.6 KiB
Python
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)
|