Skip to content

Add Condition.safe_expr for opt-in restricted AST evaluation#820

Open
elijahbenizzy wants to merge 2 commits into
mainfrom
feature/condition-safe-expr
Open

Add Condition.safe_expr for opt-in restricted AST evaluation#820
elijahbenizzy wants to merge 2 commits into
mainfrom
feature/condition-safe-expr

Conversation

@elijahbenizzy

Copy link
Copy Markdown
Contributor

Closes #817.

Summary

Adds a new public API Condition.safe_expr() (and a top-level safe_expr re-export from burr.core for symmetry with expr) that evaluates a string expression under a strict AST allowlist, intended for callers that want to accept expression strings from less-trusted sources (dashboard rule editors, YAML-driven graph definitions, configuration files).

Existing Condition.expr() is unchanged — it remains a developer-authored, full-power eval surface. The trust-model contrast is captured in both functions' docstrings, with expr() now pointing readers at safe_expr() for the untrusted-input case (cross-reference docstring update lands in this PR).

Safety argument

The validated AST is interpreted directly — there is no eval() or compile() call on the tree. The pipeline is:

  1. ast.parse(expr, mode="eval") (parse only — does not execute).
  2. _SafeExprValidator(ast.NodeVisitor) walks the tree. generic_visit is a hard-rejection catch-all: any node type not explicitly opted in raises ValueError at parse/validate time, not at runtime.
  3. _SafeExprInterpreter walks the validated tree via per-node-type methods and produces a value.
  4. Builtins available inside expressions are looked up in a hard-coded 11-name dict (_SAFE_EXPR_BUILTINS), not builtins.__dict__.

Because no eval() is ever invoked, the safety surface does not depend on CPython's eval semantics (no __builtins__ auto-injection concern, no globals/locals juggling).

What's allowed

Constants; Name; Attribute access (but any __dunder__ attribute access is rejected — closes the (0).__class__.__bases__[0].__subclasses__() escape); Subscript; Compare (full set including Is/IsNot/In/NotIn); BoolOp (and/or); UnaryOp (Not, USub, UAdd); arithmetic BinOp (+ - * / // % **); literal containers (Tuple, List, Set, Dict); Call only to the allowlist {len, abs, min, max, sum, all, any, str, int, float, bool}.

What's rejected at parse time

Lambda, IfExp, all comprehensions, Import/ImportFrom, Yield/YieldFrom/Await, NamedExpr (walrus), any Call to a non-allowlisted name, any Attribute access starting with __. Anything not explicitly opted in falls through to generic_visit which raises.

Tests

47 new tests in tests/core/test_action.py (≈19 positive — grammar coverage, builtins, determinism, edge cases — and ≈28 negative, including 7 explicit attack-pattern tests like __import__("os").system(...), (0).__class__.__bases__[0].__subclasses__(), open("/etc/passwd").read(), lambda: 1, comprehensions).

Full test file: 154 passed in 1.37s.

Open questions for reviewers

  1. Builtin allowlist is strict per the issue — round, sorted, set/list/tuple/dict constructors, range are intentionally not included. range in particular is hazardous (DoS via sum(range(10**12))). Worth confirming this is the desired posture for the initial cut.
  2. Attribute access is allowed broadly on non-dunder names. A future opt-out (safe_expr(..., allow_attributes=False)) for stricter deployments would be a small add.
  3. Is / IsNot are allowed per the issue but are a string/int-identity footgun. Worth flagging in the docstring or leaving for a future hardening pass.
  4. Resource bounds — no timeout, depth, or memory limits. safe_expr("x ** y ** y") with attacker-controlled x, y can still hang or OOM. Out of scope for this PR; worth a follow-up issue.
  5. safe_when (kwarg form) symmetric to safe_expr — not in the issue. Flagging for triage.

)

Introduce Condition.safe_expr() as the opt-in safe sibling of Condition.expr().
Where expr() uses eval() (acceptable for developer-authored strings, unsafe
for less-trusted input), safe_expr() parses the expression, validates every
AST node against a tight allowlist at call time, and then interprets the
validated tree directly. eval()/compile() are never invoked on the parsed
tree -- the safety argument depends on this.

Allowed grammar: constants, Name lookup (resolved against state), Attribute
access (dunder names rejected -- closes __class__.__bases__.__subclasses__()),
Subscript, Compare, BoolOp, UnaryOp, arithmetic BinOp, literal containers,
and Call only to a whitelist of safe builtins (len/abs/min/max/sum/all/any/
str/int/float/bool). Everything else -- lambdas, comprehensions, IfExp,
walrus, f-strings, bitwise ops, arbitrary calls, bytes/Ellipsis constants --
raises ValueError at safe_expr() call time, before the Condition is built.

Backward compatible: Condition.expr is unchanged. safe_expr is exported as
burr.core.safe_expr alongside expr. expr's docstring now cross-references
safe_expr for the untrusted-input use case.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@elijahbenizzy elijahbenizzy requested a review from skrawcz June 22, 2026 19:26
@github-actions github-actions Bot added the area/core Application, State, Graph, Actions label Jun 22, 2026

@skrawcz skrawcz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/core Application, State, Graph, Actions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Provide opt-in safe AST evaluator for Condition.expr() to support untrusted expression sources

2 participants