[PR] Tighten Condition.expr documentation; remove misleading empty globals dict [burr]

["elijahbenizzy (via GitHub)" <gi...@apache.org>]

elijahbenizzy opened a new pull request, #819:
URL: https://github.com/apache/burr/pull/819

   ## 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 (#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 #817 lands and the two surfaces are co-documented, a small regression test asserting `Condition.expr("len(x) > 0")` works would be cheap insurance.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@burr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Tighten Condition.expr documentation; remove misleading empty globals dict [burr]

["elijahbenizzy (via GitHub)" <gi...@apache.org>]

elijahbenizzy merged PR #819:
URL: https://github.com/apache/burr/pull/819


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@burr.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org