fix: Restrict eval() in condition engine to prevent code injection#89
Conversation
The workflow condition evaluator used eval() with only a bare except
to handle failures, allowing arbitrary Python code execution via
attacker-controlled workflow expressions.
Replace with a sandboxed eval(__builtins__={}) and an allowlist
regex that rejects any expression containing dunder attributes,
import, exec, open, or introspection builtins.
Fixes: Code injection in agent_flow/engine/executor.py line 1203
Signed-off-by: FailSafe Researcher <joshua@getfailsafe.com>
The previous fix used a keyword blocklist + eval(__builtins__={}) which
is an incomplete sandbox — blocklists can be bypassed and the approach
gives a false sense of security.
Replace with an AST-based whitelist that only permits literals, arithmetic,
comparison, and boolean operators (_SAFE_AST_NODES). Any other AST node
type (calls, attribute access, imports, subscripts) is rejected before
eval() is ever called. Rejection is logged with the offending node type.
Also:
- Collapse duplicate if/else branches into a single line
- Accept 'javascript' as a deprecated alias for backwards compatibility
- Remove import-inside-loop antipattern (ast imported at module level)
Signed-off-by: FailSafe Researcher <joshua@getfailsafe.com>
Two bugs found during exploit testing: - ast.Load context node was missing from _SAFE_AST_NODES, causing list and tuple literals to be incorrectly rejected (false positive) - eval(compile(...)) was not wrapped in try/except, so arithmetic errors like ZeroDivisionError propagated up the call stack (DoS vector) Both are now fixed: ast.Load added (safe — ast.Name is still excluded, so variable name lookup is still blocked), and the eval call is wrapped in a broad except that returns False on any runtime error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: FailSafe Researcher <joshua@getfailsafe.com>
Joshua-Medvinsky
force-pushed
the
fix/restrict-eval-in-condition-engine
branch
from
10c4eb5 to
58bd0dd
Compare
|
Hi maintainers — friendly follow-up on this security hardening PR. It has been open for a while with no reviewer feedback, so I wanted to resurface it. Happy to rebase, adjust the approach, add tests, or split the change differently if that would make review easier. Thanks! |
|
Hi — this security PR has been open for 40 days without maintainer response. Per our responsible disclosure policy, I'll be closing this PR shortly. I may resubmit via a bounty platform if appropriate. Thanks for your time. |
|
Closing per responsible disclosure policy: 35+ days open with no maintainer response. May resubmit via bounty platform. |
|
This security fix has been open for 30+ days. Per our responsible disclosure timeline, we'd like to move toward resolution. We can: (1) rebase and adjust the patch, (2) reach out via security@ email, or (3) resubmit via a bounty platform. Please let us know your preference. |
2 participants
Summary
The workflow condition evaluator in `agent_flow/engine/executor.py` uses Python's `eval()` to process condition expressions that include user-controlled input (via `$input` substitution). The original call had no sandboxing and used a bare `except` block, silently swallowing errors.
Vulnerable code (original)
```python
line 1203 — runs with full builtins, no restrictions
result = bool(eval(expr))
```
An attacker controlling a workflow's input data can craft:
```
$input == import('os').system('id')
```
to achieve arbitrary code execution with the privileges of the server process.
Fix — AST whitelisting
Replaced with a sandboxed evaluator that parses the expression into an AST first, walks every node, and rejects anything outside an explicit whitelist of safe node types (literals, arithmetic, comparisons, boolean ops). Only if the AST is clean does `eval()` run — with `builtins` stripped:
```python
_SAFE_AST_NODES = (
ast.Expression, ast.BoolOp, ast.Compare, ast.BinOp, ast.UnaryOp,
ast.Constant, ast.And, ast.Or, ast.Not,
ast.Eq, ast.NotEq, ast.Lt, ast.LtE, ast.Gt, ast.GtE,
ast.In, ast.NotIn, ast.Add, ast.Sub, ast.Mult, ast.Div,
ast.Mod, ast.UAdd, ast.USub, ast.List, ast.Tuple,
ast.Load, # context marker for list/tuple literals — safe; ast.Name is still excluded
)
def _safe_eval_expr(expr: str) -> bool:
try:
tree = ast.parse(expr, mode='eval')
except SyntaxError:
return False
for node in ast.walk(tree):
if not isinstance(node, _SAFE_AST_NODES):
logger.warning("Condition expression rejected — unsafe AST node: %s in %r", type(node).name, expr)
return False
try:
return bool(eval(compile(tree, '', 'eval'), {"builtins": {}}))
except Exception:
return False
```
Why whitelist not blocklist: keyword blocklists can be bypassed via string encoding, chr() calls, or attribute access chains. AST whitelisting is structurally closed — any construct not on the list is rejected regardless of encoding.
What is blocked
Verified against 16 test cases:
Compatibility
Impact
Critical — Remote code execution via attacker-controlled workflow condition expressions.