[IMP] safe_eval: simplify API

This commit simplifies `safe_eval` which, up until now, allowed passing
global and local namespaces. There is no usecase for this anymore. We
want safe_eval exec mode to behave like a top-level module scope.

`nocopy` is not needed anymore either. It's a relic of when the context
could be dynamic (e.g. for already deleted `RecordDictWrapper` or
`QWebContext`).

Passed in context is always a dict. It will always be mutated with the
local eval namespace dict after evaluation.

This all makes `safe_eval` API less confusing.

See: odoo/enterprise#83818
See: odoo/upgrade-util#265

task-4378806

Author: antonrom1

addons/account_tax_python/models/account_tax.py

@@ -144,13 +144,7 @@ def _eval_tax_amount_formula(self, raw_base, evaluation_context):
             'max': max,
         }
         try:
-            return safe_eval(
-                self.formula_decoded_info['py_formula'],
-                globals_dict=formula_context,
-                locals_dict={},
-                locals_builtins=False,
-                nocopy=True,
-            )
+            return safe_eval(self.formula_decoded_info['py_formula'], formula_context)
         except ZeroDivisionError:
             return 0.0
 

addons/account_test/report/report_account_test.py

@@ -35,17 +35,17 @@ def order_columns(item, cols=None):
                 cols = list(item)
             return [(col, item.get(col)) for col in cols if col in item]
 
-        localdict = {
+        context = {
             'cr': self.env.cr,
             'uid': self.env.uid,
             'reconciled_inv': reconciled_inv,  # specific function used in different tests
             'result': None,  # used to store the result of the test
             'column_order': None,  # used to choose the display order of columns (in case you are returning a list of dict)
             '_': lambda *a, **kw: self.env._(*a, **kw),  # pylint: disable=E8502,
         }
-        safe_eval(code_exec, localdict, mode="exec", nocopy=True)
-        result = localdict['result']
-        column_order = localdict.get('column_order', None)
+        safe_eval(code_exec, context, mode="exec")
+        result = context['result']
+        column_order = context.get('column_order')
 
         if not isinstance(result, (tuple, list, set)):
             result = [result]

addons/gamification/models/gamification_goal.py

@@ -163,7 +163,7 @@ def update_goal(self):
                         'time': time,
                     }
                     code = definition.compute_code.strip()
-                    safe_eval(code, cxt, mode="exec", nocopy=True)
+                    safe_eval(code, cxt, mode="exec")
                     # the result of the evaluated codeis put in the 'result' local variable, propagated to the context
                     result = cxt.get('result')
                     if isinstance(result, (float, int)):

odoo/addons/base/models/ir_actions.py

@@ -907,7 +907,7 @@ def unlink_action(self):
         return True
 
     def _run_action_code_multi(self, eval_context):
-        safe_eval(self.code.strip(), eval_context, mode="exec", nocopy=True, filename=str(self))  # nocopy allows to return 'action'
+        safe_eval(self.code.strip(), eval_context, mode="exec", filename=str(self))
         return eval_context.get('action')
 
     def _run_action_multi(self, eval_context=None):

odoo/addons/base/models/ir_model.py

@@ -47,7 +47,8 @@
 
 def make_compute(text, deps):
     """ Return a compute function from its code body and dependencies. """
-    func = lambda self: safe_eval(text, SAFE_EVAL_BASE, {'self': self}, mode="exec")
+    def func(self):
+        return safe_eval(text, SAFE_EVAL_BASE | {'self': self}, mode="exec")
     deps = [arg.strip() for arg in deps.split(",")] if deps else []
     return api.depends(*deps)(func)
 

odoo/addons/base/tests/test_base.py

@@ -34,17 +34,17 @@ def test_expr_eval_opcodes(self):
             self.assertEqual(expr_eval(expr), expected)
 
     def test_safe_eval_opcodes(self):
-        for expr, locals_dict, expected in [
+        for expr, context, expected in [
             ('[x for x in (1,2)]', {}, [1, 2]),  # LOAD_FAST_AND_CLEAR
             ('list(x for x in (1,2))', {}, [1, 2]),  # END_FOR, CALL_INTRINSIC_1
             ('v if v is None else w', {'v': False, 'w': 'foo'}, 'foo'),  # POP_JUMP_IF_NONE
             ('v if v is not None else w', {'v': None, 'w': 'foo'}, 'foo'),  # POP_JUMP_IF_NOT_NONE
             ('{a for a in (1, 2)}', {}, {1, 2}),  # RERAISE
         ]:
-            self.assertEqual(safe_eval(expr, locals_dict=locals_dict), expected)
+            self.assertEqual(safe_eval(expr, context), expected)
 
     def test_safe_eval_exec_opcodes(self):
-        for expr, locals_dict, expected in [
+        for expr, context, expected in [
             ("""
                 def f(v):
                     if v:
@@ -53,8 +53,47 @@ def f(v):
                 result = f(42)
             """, {}, 1),  # LOAD_FAST_CHECK
         ]:
-            safe_eval(dedent(expr), locals_dict=locals_dict, mode="exec", nocopy=True)
-            self.assertEqual(locals_dict['result'], expected)
+            safe_eval(dedent(expr), context, mode="exec")
+            self.assertEqual(context['result'], expected)
+
+    def test_safe_eval_strips(self):
+        # cpython strips spaces and tabs by deafult since 3.10
+        # https://github.com/python/cpython/commit/e799aa8b92c195735f379940acd9925961ad04ec
+        # but we need to strip all whitespaces
+        for expr, expected in [
+            # simple ascii
+            ("\n 1 + 2", 3),
+            ("\n\n\t 1 + 2 \n", 3),
+            ("   1 + 2", 3),
+            ("1 + 2   ", 3),
+
+            # Unicode (non-ASCII spaces)
+            ("\u00A01 + 2\u00A0", 3),  # nbsp
+        ]:
+            self.assertEqual(safe_eval(expr), expected)
+
+    def test_runs_top_level_scope(self):
+        # when we define var in top-level scope, it should become available in locals and globals
+        # such that f's frame will be able to access var too.
+        expr = dedent("""
+        var = 1
+        def f():
+            return var
+        f()
+        """)
+        safe_eval(expr, mode="exec")
+        safe_eval(expr, context={}, mode="exec")
+
+    def test_safe_eval_ctx_mutation(self):
+        # simple eval also has side-effect on context
+        expr = '(answer := 42)'
+        context = {}
+        self.assertEqual(safe_eval(expr, context), 42)
+        self.assertEqual(context, {'answer': 42})
+
+    def test_safe_eval_ctx_no_builtins(self):
+        ctx = {'__builtins__': {'max': min}}
+        self.assertEqual(safe_eval('max(1, 2)', ctx), 2)
 
     def test_01_safe_eval(self):
         """ Try a few common expressions to verify they work with safe_eval """

odoo/tools/convert.py

@@ -26,15 +26,11 @@
 from .misc import file_open, file_path, SKIPPED_ELEMENT_TYPES
 from odoo.exceptions import ValidationError
 
-from .safe_eval import safe_eval as s_eval, pytz, time
+from .safe_eval import safe_eval, pytz, time
 
 _logger = logging.getLogger(__name__)
 
 
-def safe_eval(expr, ctx={}):
-    return s_eval(expr, ctx, nocopy=True)
-
-
 class ParseError(Exception):
     ...
 

odoo/tools/safe_eval.py

@@ -239,6 +239,7 @@ def assert_valid_codeobj(allowed_codes, code_obj, expr):
         if isinstance(const, CodeType):
             assert_valid_codeobj(allowed_codes, const, 'lambda')
 
+
 def test_expr(expr, allowed_codes, mode="eval", filename=None):
     """test_expr(expression, allowed_codes[, mode[, filename]]) -> code_object
 
@@ -344,18 +345,35 @@ def expr_eval(expr):
     'zip': zip,
     'Exception': Exception,
 }
-def safe_eval(expr, globals_dict=None, locals_dict=None, mode="eval", nocopy=False, locals_builtins=False, filename=None):
-    """safe_eval(expression[, globals[, locals[, mode[, nocopy]]]]) -> result
 
-    System-restricted Python expression evaluation
+
+_BUBBLEUP_EXCEPTIONS = (
+    odoo.exceptions.UserError,
+    odoo.exceptions.RedirectWarning,
+    werkzeug.exceptions.HTTPException,
+    OperationalError,  # let auto-replay of serialized transactions work its magic
+    ZeroDivisionError,
+)
+
+
+def safe_eval(expr, /, context=None, *, mode="eval", filename=None):
+    """System-restricted Python expression evaluation
 
     Evaluates a string that contains an expression that mostly
     uses Python constants, arithmetic expressions and the
     objects directly provided in context.
 
     This can be used to e.g. evaluate
-    an OpenERP domain expression from an untrusted source.
-
+    a domain expression from an untrusted source.
+
+    :param expr: The Python expression (or block, if ``mode='exec'``) to evaluate.
+    :type expr: string | bytes
+    :param context: Namespace available to the expression.
+                    This dict will be mutated with any variables created during
+                    evaluation
+    :type context: dict
+    :param mode: ``exec`` or ``eval``
+    :type mode: str
     :param filename: optional pseudo-filename for the compiled expression,
                      displayed for example in traceback frames
     :type filename: string
@@ -367,48 +385,29 @@ def safe_eval(expr, globals_dict=None, locals_dict=None, mode="eval", nocopy=Fal
     if type(expr) is CodeType:
         raise TypeError("safe_eval does not allow direct evaluation of code objects.")
 
-    # prevent altering the globals/locals from within the sandbox
-    # by taking a copy.
-    if not nocopy:
-        # isinstance() does not work below, we want *exactly* the dict class
-        if (globals_dict is not None and type(globals_dict) is not dict) \
-                or (locals_dict is not None and type(locals_dict) is not dict):
-            _logger.warning(
-                "Looks like you are trying to pass a dynamic environment, "
-                "you should probably pass nocopy=True to safe_eval().")
-        if globals_dict is not None:
-            globals_dict = dict(globals_dict)
-        if locals_dict is not None:
-            locals_dict = dict(locals_dict)
-
-    check_values(globals_dict)
-    check_values(locals_dict)
-
-    if globals_dict is None:
-        globals_dict = {}
-
-    globals_dict['__builtins__'] = dict(_BUILTINS)
-    if locals_builtins:
-        if locals_dict is None:
-            locals_dict = {}
-        locals_dict.update(_BUILTINS)
+    assert context is None or type(context) is dict, "Context must be a dict"
+
+    check_values(context)
+
+    globals_dict = dict(context or {}, __builtins__=dict(_BUILTINS))
+
     c = test_expr(expr, _SAFE_OPCODES, mode=mode, filename=filename)
     try:
-        return unsafe_eval(c, globals_dict, locals_dict)
-    except odoo.exceptions.UserError:
-        raise
-    except odoo.exceptions.RedirectWarning:
-        raise
-    except werkzeug.exceptions.HTTPException:
-        raise
-    except OperationalError:
-        # Do not hide PostgreSQL low-level exceptions, to let the auto-replay
-        # of serialized transactions work its magic
-        raise
-    except ZeroDivisionError:
+        # empty locals dict makes the eval behave like top-level code
+        return unsafe_eval(c, globals_dict, None)
+
+    except _BUBBLEUP_EXCEPTIONS:
         raise
+
     except Exception as e:
         raise ValueError('%r while evaluating\n%r' % (e, expr))
+
+    finally:
+        if context is not None:
+            del globals_dict['__builtins__']
+            context.update(globals_dict)
+
+
 def test_python_expr(expr, mode="eval"):
     try:
         test_expr(expr, _SAFE_OPCODES, mode=mode)