Tighten Condition.expr documentation; remove misleading empty globals dict#819
Open
elijahbenizzy wants to merge 1 commit into
Open
Tighten Condition.expr documentation; remove misleading empty globals dict#819elijahbenizzy wants to merge 1 commit into
elijahbenizzy wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two small, behavior-preserving changes to
Condition.expr()inburr/core/action.py: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).Replace
globals={}withglobals=Nonein theeval()call. Functionally identical in CPython 3 (an empty globals dict still receives__builtins__auto-injection by the interpreter —len(...)etc. continue to work), butNonedoesn't visually read as a sandbox. A future maintainer reading the line is less likely to assumeCondition.exprdoes any restriction. Added a# NOTEimmediately above the call repeating the point inline.The behavior of
Condition.expris unchanged.tests/core/test_action.pypasses 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.exprpopulates theCondition.readsset by walkingast.Namenodes 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.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 assertingCondition.expr("len(x) > 0")works would be cheap insurance.