fix: resolve bare names with the same precedence in the restricted and trusted evaluators

docs/io/formats.md

@@ -195,6 +195,36 @@ property or attribute is *read* and a **method** is *called* (with dependency in
 
 `cond` and `unless` also accept a list; the transition is taken only when every entry holds.
 
+A bare name resolves to a datamodel variable or a model attribute (including a property) first
+and to the event's keyword arguments only when the model has no attribute of that name, in
+both trusted and restricted mode. Under `trusted=True`, model methods and non-instance
+attributes named like a Python builtin (`id`, `type`, ...) are not resolved by bare name. A
+payload key that collides with a datamodel variable is still reachable as `_event.data.<name>`:
+
+```py
+>>> sc = load(
+...     """
+...     datamodel:
+...       - {id: limit, expr: "10"}
+...     states:
+...       idle:
+...         initial: true
+...         transitions:
+...           - {event: check, target: within, cond: "limit == 10 and _event.data.limit == 99"}
+...           - {event: check, target: other}
+...       within: {final: true}
+...       other: {final: true}
+...     """,
+...     format="yaml",
+...     validate=True,
+... )
+>>> sm = sc()
+>>> _ = sm.send("check", limit=99)
+>>> sorted(sm.configuration_values)
+['within']
+
+```
+
 ## Actions
 
 Five positions carry behaviour: a state's `enter` and `exit`, and a transition's `before`,

docs/releases/3.3.0.md

@@ -52,6 +52,22 @@ on the old behavior can still behave differently or fail after upgrading:
   loop, as before, or, when the invocation itself processes `done.invoke`, through the
   channels above, where it used to become `error.execution`. See
   {ref}`release-3.3.0-done-invoke`.
+- In an SCXML, JSON or YAML document loaded with the default restricted evaluator, a bare
+  name that is both a datamodel variable or model attribute and a `send()` keyword argument
+  now resolves to the model. It used to resolve to the keyword argument. The payload value
+  is still `_event.data.<name>`. This includes a bare method name in a guard: the method is
+  now called even when the event carries a keyword argument of the same name. See
+  {ref}`release-3.3.0-bare-names`.
+- A model with a catch-all `__getattr__` (a proxy, a `Mock`-like object) now answers every
+  public bare name under both evaluators (under `trusted=True`, every name not named like a
+  Python builtin), so payload keys are reachable only as `_event.data.<name>`. See
+  {ref}`release-3.3.0-bare-names`.
+- In a document loaded with `trusted=True`, a bare name now also resolves to a property,
+  class attribute or slot of the model, ahead of a `send()` keyword argument of the same
+  name. Only instance attributes used to resolve, so these names resolved to the keyword
+  argument or raised `NameError`. Methods defined on the model's class and non-instance
+  attributes named like a Python builtin (`len`, `type`, ...) are still not resolved by bare
+  name. See {ref}`release-3.3.0-bare-names`.
 
 ## Deprecations in 3.3.0
 
@@ -134,3 +150,25 @@ handler. An error raised by a handler's `on_finalize` goes through the same erro
 See [](../invoke.md#error-handling).
 
 [#698](https://github.com/fgmacedo/python-statemachine/pull/698).
+
+(release-3.3.0-bare-names)=
+
+### Bare names in document expressions
+
+A bare name in a `cond` or a value expression of an SCXML, JSON or YAML document can come
+from the datamodel, from an attribute of the bound model or from the keyword arguments of
+`send()`. The two evaluators disagreed when a name came from both: with a datamodel item
+`value = 1` and `send("go", value=5)`, `cond: "value == 5"` held under the default
+restricted evaluator, which looked the keyword arguments up first, and failed under
+`trusted=True`, where the datamodel won. The trusted evaluator also read only the model's
+instance attributes, so a property, class attribute or slot did not resolve by bare name.
+
+Both evaluators now resolve a bare name to a datamodel variable or a public model attribute
+(instance attribute, property, class attribute, slot) first, and to the keyword arguments
+only when the model has no attribute of that name. A payload key that collides with a
+datamodel variable is still reachable as `_event.data.<name>`. Two differences remain under
+`trusted=True`: a method defined on the model's class is not resolved by bare name, and neither is a non-instance
+attribute named like a Python builtin, which keeps resolving to the payload or to the
+builtin. See [](../io/formats.md#guards).
+
+[#695](https://github.com/fgmacedo/python-statemachine/pull/695).

statemachine/io/evaluators.py

@@ -28,10 +28,12 @@
 """
 
 import ast
+import builtins
 import html
 import re
 from collections.abc import Callable
 from inspect import isawaitable
+from inspect import isroutine
 from typing import Any
 from typing import Protocol
 
@@ -76,32 +78,38 @@ def _resolvable_model_attr(name: str, model) -> bool:
     Engine-protected names (``machine``, ``event``, system variables, …) and any
     private/dunder name (leading underscore) never resolve off the model, mirroring the
     dotted-access guard in :func:`statemachine.spec_parser.build_attribute`. System
-    variables stay reachable because they are provided via the call kwargs, which are
-    consulted before this. This keeps a semi-trusted document from reading the model's
-    internals (``_secret``) or walking the object graph (``__class__``).
+    variables stay reachable because they are provided via the call kwargs. This keeps a
+    semi-trusted document from reading the model's internals (``_secret``) or walking the
+    object graph (``__class__``).
     """
     return name not in protected_attrs and not name.startswith("_") and hasattr(model, name)
 
 
+def _kwarg(name: str, kwargs: dict) -> Any:
+    if name not in _event_data_kwargs and name in kwargs:
+        return kwargs[name]
+    raise NameError(f"name '{name}' is not defined")
+
+
 def variable_hook(name: str) -> Callable:
     """Resolve a datamodel name at call time.
 
-    Looks the name up in the call kwargs first (SCXML system variables such as
-    ``_event``, ``_name``), then falls back to a (non-protected, non-private) attribute of
-    ``machine.model``. Names in :data:`~statemachine.event._event_data_kwargs` (``machine``,
-    ``model``, ``event``, ``transition``, ...) are the live engine-capability objects; they
-    are **withheld** from the kwargs lookup so an untrusted expression cannot obtain the
-    engine and pivot through it onto shared classes (GHSA-v3qq-3xvg-m77g). The ``_``-prefixed
-    system variables are not engine objects and stay resolvable.
+    Looks the name up as a (non-protected, non-private) attribute of ``machine.model``
+    first, so the datamodel shadows an event keyword argument of the same name (the payload
+    stays reachable as ``_event.data.<name>``). It then falls back to the call kwargs, which
+    carry the SCXML system variables (``_event``, ``_name``) and the event payload. Names in
+    :data:`~statemachine.event._event_data_kwargs` (``machine``, ``model``, ``event``,
+    ``transition``, ...) are the live engine-capability objects; they are **withheld** from
+    the kwargs lookup so an untrusted expression cannot obtain the engine and pivot through
+    it onto shared classes (GHSA-v3qq-3xvg-m77g). The ``_``-prefixed system variables are not
+    engine objects and stay resolvable.
     """
 
     def resolver(*args, **kwargs):
-        if name not in _event_data_kwargs and name in kwargs:
-            return kwargs[name]
         model = kwargs["machine"].model
         if _resolvable_model_attr(name, model):
             return getattr(model, name)
-        raise NameError(f"name '{name}' is not defined")
+        return _kwarg(name, kwargs)
 
     resolver.__name__ = name
     return resolver
@@ -117,20 +125,46 @@ def cond_variable_hook(name: str) -> Callable:
     """
 
     def resolver(*args, **kwargs):
-        if name not in _event_data_kwargs and name in kwargs:
-            return kwargs[name]
         model = kwargs["machine"].model
         if _resolvable_model_attr(name, model):
             value = getattr(model, name)
             if callable(value):
                 return callable_method(value)(*args, **kwargs)
             return value
-        raise NameError(f"name '{name}' is not defined")
+        return _kwarg(name, kwargs)
 
     resolver.__name__ = name
     return resolver
 
 
+class _TrustedNamespace(dict):
+    """``eval`` locals that resolve a bare name the way the restricted evaluator does.
+
+    The model shadows the call kwargs. Instance attributes resolve as before; any other
+    attribute (property, class attribute, slot) resolves through the same boundary as
+    :func:`_resolvable_model_attr`, unless it is named like a builtin. Methods are left out:
+    the restricted guard dialect calls a bare method name, which ``eval`` cannot reproduce
+    per name.
+    """
+
+    def __init__(self, model, kwargs: dict, **overrides):
+        super().__init__(kwargs)
+        self.update(overrides)
+        self._model = model
+        self._overrides = overrides
+
+    def __getitem__(self, name: str) -> Any:
+        if name not in self._overrides and name not in protected_attrs:
+            instance_attrs = getattr(self._model, "__dict__", {})
+            if name in instance_attrs:
+                return instance_attrs[name]
+            if name not in builtins.__dict__ and _resolvable_model_attr(name, self._model):
+                value = getattr(self._model, name)
+                if not isroutine(value):
+                    return value
+        return super().__getitem__(name)
+
+
 def _eval(expr: str, **kwargs) -> Any:
     """Evaluate an expression as arbitrary Python (``trusted=True`` path only).
 
@@ -140,14 +174,8 @@ def _eval(expr: str, **kwargs) -> Any:
         the document is loaded with ``trusted=True``.
     """
     if "machine" in kwargs:
-        kwargs.update(
-            **{
-                k: v
-                for k, v in kwargs["machine"].model.__dict__.items()
-                if k not in protected_attrs
-            }
-        )
-        kwargs["In"] = InState(kwargs["machine"])
+        machine = kwargs["machine"]
+        return eval(expr, {}, _TrustedNamespace(machine.model, kwargs, In=InState(machine)))
     return eval(expr, {}, kwargs)
 
 

tests/io/test_name_resolution.py

@@ -0,0 +1,172 @@
+"""Bare-name resolution in native expressions is the same under both evaluators.
+
+A name in a ``cond`` or in a value expression can come from the datamodel, from an attribute
+of the bound model or from the keyword arguments passed to ``send``. The restricted
+(``trusted=False``) and the trusted evaluators must resolve it to the same value.
+"""
+
+import pytest
+from statemachine.io import load
+
+DOCUMENT = """
+datamodel:
+  - {{id: seen, expr: "None"}}
+{datamodel}
+states:
+  wait:
+    initial: true
+    transitions:
+      - event: go
+        target: matched
+        cond: "{expr} == {expected}"
+        on:
+          - assign: {{location: seen, expr: "{expr}"}}
+      - {{event: go, target: unmatched}}
+  matched: {{final: true}}
+  unmatched: {{final: true}}
+"""
+
+VALUE_FROM_DATAMODEL = '  - {id: value, expr: "1"}'
+
+
+class InstanceAttribute:
+    def __init__(self):
+        self.value = 3
+
+
+class Property:
+    @property
+    def value(self):
+        return 3
+
+
+class ClassAttribute:
+    value = 3
+
+
+class Slots:
+    __slots__ = ("value", "state", "seen")
+
+    def __init__(self):
+        self.value = 3
+
+
+MODEL_KINDS = [
+    pytest.param(kind, id=kind.__name__)
+    for kind in (InstanceAttribute, Property, ClassAttribute, Slots)
+]
+
+
+@pytest.fixture(params=[False, True], ids=["restricted", "trusted"])
+def trusted(request):
+    return request.param
+
+
+def _run(trusted, *, expected, expr="value", datamodel="", model=None, **payload):
+    sc = load(
+        DOCUMENT.format(datamodel=datamodel, expr=expr, expected=expected),
+        format="yaml",
+        trusted=trusted,
+        validate=True,
+    )
+    sm = sc() if model is None else sc(model=model())
+    sm.send("go", **payload)
+    return sm
+
+
+@pytest.mark.parametrize(
+    ("datamodel", "payload", "expected"),
+    [
+        pytest.param(VALUE_FROM_DATAMODEL, {}, 1, id="datamodel-only"),
+        pytest.param("", {"value": 5}, 5, id="kwarg-only"),
+    ],
+)
+def test_name_from_a_single_source(trusted, datamodel, payload, expected):
+    sm = _run(trusted, expected=expected, datamodel=datamodel, **payload)
+
+    assert sm.configuration_values == {"matched"}
+    assert sm.model.seen == expected
+
+
+@pytest.mark.parametrize("model", MODEL_KINDS)
+def test_name_from_the_model_only(trusted, model):
+    sm = _run(trusted, expected=3, model=model)
+
+    assert sm.configuration_values == {"matched"}
+    assert sm.model.seen == 3
+
+
+def test_event_data_reaches_the_payload(trusted):
+    sm = _run(
+        trusted, expected=5, expr="_event.data.value", datamodel=VALUE_FROM_DATAMODEL, value=5
+    )
+
+    assert sm.configuration_values == {"matched"}
+    assert sm.model.seen == 5
+
+
+def test_datamodel_shadows_the_payload(trusted):
+    sm = _run(trusted, expected=1, datamodel=VALUE_FROM_DATAMODEL, value=5)
+
+    assert sm.configuration_values == {"matched"}
+    assert sm.model.seen == 1
+
+
+@pytest.mark.parametrize("model", MODEL_KINDS)
+def test_model_attribute_shadows_the_payload(trusted, model):
+    sm = _run(trusted, expected=3, model=model, value=5)
+
+    assert sm.configuration_values == {"matched"}
+    assert sm.model.seen == 3
+
+
+def test_assigned_value_shadows_the_payload(trusted):
+    sc = load(
+        """
+        datamodel:
+          - {id: value, expr: "1"}
+        states:
+          wait:
+            initial: true
+            transitions:
+              - event: update
+                on:
+                  - assign: {location: value, expr: "7"}
+              - {event: go, target: matched, cond: "value == 7"}
+              - {event: go, target: unmatched}
+          matched: {final: true}
+          unmatched: {final: true}
+        """,
+        format="yaml",
+        trusted=trusted,
+        validate=True,
+    )
+    sm = sc()
+    sm.send("update")
+    sm.send("go", value=5)
+
+    assert sm.configuration_values == {"matched"}
+
+
+class Method:
+    def value(self):
+        return 3
+
+
+def test_trusted_mode_does_not_resolve_a_model_method_by_bare_name():
+    sm = _run(True, expected=5, model=Method, value=5)
+
+    assert sm.configuration_values == {"matched"}
+    assert sm.model.seen == 5
+
+
+class BuiltinNamedClassAttribute:
+    len = 0
+    value = 3
+
+
+def test_trusted_mode_keeps_builtins_over_model_class_attributes():
+    sm = _run(True, expected=1, expr="len([value])", model=BuiltinNamedClassAttribute)
+
+    assert sm.configuration_values == {"matched"}
+    assert sm.model.seen == 1