* 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>
