Skip to content

Tighten Condition.expr documentation; remove misleading empty globals dict#819

Open
elijahbenizzy wants to merge 1 commit into
mainfrom
improve/condition-expr-docs
Open

Tighten Condition.expr documentation; remove misleading empty globals dict#819
elijahbenizzy wants to merge 1 commit into
mainfrom
improve/condition-expr-docs

Conversation

@elijahbenizzy

Copy link
Copy Markdown
Contributor

Summary

Two small, behavior-preserving changes to Condition.expr() in burr/core/action.py:

  1. Rewrite the docstring to make the trust model unambiguous. The previous text said "Do not accept expressions generated from user-inputted text, this has the potential to be unsafe" — accurate but easy to overlook. The new docstring uses a .. warning:: directive (matching local Sphinx style), gives concrete examples of what attacker-controllable input would mean (e.g. __import__("os").system(...)), and points callers who need to accept less-trusted expression sources at the issue tracking the opt-in safe AST evaluator (Provide opt-in safe AST evaluator for Condition.expr() to support untrusted expression sources #817).

  2. Replace globals={} with globals=None in the eval() call. Functionally identical in CPython 3 (an empty globals dict still receives __builtins__ auto-injection by the interpreter — len(...) etc. continue to work), but None doesn't visually read as a sandbox. A future maintainer reading the line is less likely to assume Condition.expr does any restriction. Added a # NOTE immediately above the call repeating the point inline.

The behavior of Condition.expr is unchanged. tests/core/test_action.py passes 107/107.

Why these changes

Condition.expr() is a developer-authored-expression surface — the framework user writes the expression string themselves at graph-construction time, equivalent to writing a lambda. That's the documented contract, and nothing here changes it. The aim of this PR is just to make the contract impossible to misread: the docstring states the contract emphatically and the call site no longer looks like it's doing restriction.

For the genuine "accept expressions from less-trusted sources" use case (dashboard rule editors, YAML-driven graph definitions, etc.), the opt-in Condition.safe_expr() is tracked at #817.

Adjacent observations (out of scope here)

  • Condition.expr populates the Condition.reads set by walking ast.Name nodes only — attribute access (obj.foo) and subscripts (obj["foo"]) on state values work at eval time but don't contribute to the declared read set. Worth a separate look at some point; not a regression introduced here.
  • There is no test pinning the "builtins available via len, min, etc." behavior as intentional. Once Provide opt-in safe AST evaluator for Condition.expr() to support untrusted expression sources #817 lands and the two surfaces are co-documented, a small regression test asserting Condition.expr("len(x) > 0") works would be cheap insurance.

@elijahbenizzy elijahbenizzy requested a review from skrawcz June 22, 2026 19:20
@github-actions github-actions Bot added the area/core Application, State, Graph, Actions label Jun 22, 2026
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.

1 participant