Increase coverage of `guard_eval`

IPython/core/completer.py

@@ -996,7 +996,8 b' class Completer(Configurable):'
 
         - ``forbidden``: no evaluation of code is permitted,
         - ``minimal``: evaluation of literals and access to built-in namespace;
-          no item/attribute evaluation nor access to locals/globals,
+          no item/attribute evaluationm no access to locals/globals,
+          no evaluation of any operations or comparisons.
         - ``limited``: access to all namespaces, evaluation of hard-coded methods
           (for example: :any:`dict.keys`, :any:`object.__getattr__`,
           :any:`object.__getitem__`) on allow-listed objects (for example:

IPython/core/guarded_eval.py

@@ -108,6 +108,7 b' class EvaluationPolicy:'
             return True
 
         owner_method = _unbind_method(func)
+
         if owner_method and owner_method in self.allowed_calls:
             return True
 
@@ -127,6 +128,10 b' def _has_original_dunder_external('
         value_type = type(value)
         if type(value) == member_type:
             return True
+        if method_name == "__getattribute__":
+            # we have to short-circuit here due to an unresolved issue in
+            # `isinstance` implementation: https://bugs.python.org/issue32683
+            return False
         if isinstance(value, member_type):
             method = getattr(value_type, method_name, None)
             member_method = getattr(member_type, method_name, None)
@@ -149,7 +154,7 b' def _has_original_dunder('
 
     method = getattr(value_type, method_name, None)
 
-    if not method:
+    if method is None:
         return None
 
     if method in allowed_methods:
@@ -193,6 +198,7 b' class SelectivePolicy(EvaluationPolicy):'
             allowed_external=self.allowed_getattr_external,
             method_name="__getattr__",
         )
+
         # Many objects do not have `__getattr__`, this is fine
         if has_original_attr is None and has_original_attribute:
             return True
@@ -200,10 +206,6 b' class SelectivePolicy(EvaluationPolicy):'
         # Accept objects without modifications to `__getattr__` and `__getattribute__`
         return has_original_attr and has_original_attribute
 
-    def get_attr(self, value, attr):
-        if self.can_get_attr(value, attr):
-            return getattr(value, attr)
-
     def can_get_item(self, value, item):
         """Allow accessing `__getiitem__` of allow-listed instances unless it was not modified."""
         return _has_original_dunder(
@@ -215,20 +217,24 b' class SelectivePolicy(EvaluationPolicy):'
         )
 
     def can_operate(self, dunders: Tuple[str, ...], a, b=None):
+        objects = [a]
+        if b is not None:
+            objects.append(b)
         return all(
             [
                 _has_original_dunder(
-                    a,
+                    obj,
                     allowed_types=self.allowed_operations,
-                    allowed_methods=self._dunder_methods(dunder),
+                    allowed_methods=self._operator_dunder_methods(dunder),
                     allowed_external=self.allowed_operations_external,
                     method_name=dunder,
                 )
                 for dunder in dunders
+                for obj in objects
             ]
         )
 
-    def _dunder_methods(self, dunder: str) -> Set[Callable]:
+    def _operator_dunder_methods(self, dunder: str) -> Set[Callable]:
         if dunder not in self._operation_methods_cache:
             self._operation_methods_cache[dunder] = self._safe_get_methods(
                 self.allowed_operations, dunder
@@ -257,7 +263,7 b' class SelectivePolicy(EvaluationPolicy):'
 
 
 class _DummyNamedTuple(NamedTuple):
-    pass
+    """Used internally to retrieve methods of named tuple instance."""
 
 
 class EvaluationContext(NamedTuple):
@@ -451,12 +457,15 b' def eval_node(node: Union[ast.AST, None], context: EvaluationContext):'
                         f"not allowed in {context.evaluation} mode",
                     )
             else:
-                raise ValueError(f"Comparison `{dunder}` not supported")
+                raise ValueError(
+                    f"Comparison `{dunder}` not supported"
+                )  # pragma: no cover
         return all_true
     if isinstance(node, ast.Constant):
         return node.value
     if isinstance(node, ast.Index):
-        return eval_node(node.value, context)
+        # deprecated since Python 3.9
+        return eval_node(node.value, context)  # pragma: no cover
     if isinstance(node, ast.Tuple):
         return tuple(eval_node(e, context) for e in node.elts)
     if isinstance(node, ast.List):
@@ -477,7 +486,8 b' def eval_node(node: Union[ast.AST, None], context: EvaluationContext):'
             eval_node(node.step, context),
         )
     if isinstance(node, ast.ExtSlice):
-        return tuple([eval_node(dim, context) for dim in node.dims])
+        # deprecated since Python 3.9
+        return tuple([eval_node(dim, context) for dim in node.dims])  # pragma: no cover
     if isinstance(node, ast.UnaryOp):
         value = eval_node(node.operand, context)
         dunders = _find_dunder(node.op, UNARY_OP_DUNDERS)
@@ -490,7 +500,6 b' def eval_node(node: Union[ast.AST, None], context: EvaluationContext):'
                     type(value),
                     f"not allowed in {context.evaluation} mode",
                 )
-        raise ValueError("Unhandled unary operation:", node.op)
     if isinstance(node, ast.Subscript):
         value = eval_node(node.value, context)
         slice_ = eval_node(node.slice, context)
@@ -507,14 +516,14 b' def eval_node(node: Union[ast.AST, None], context: EvaluationContext):'
         if policy.allow_globals_access and node.id in context.globals:
             return context.globals[node.id]
         if policy.allow_builtins_access and hasattr(builtins, node.id):
-            # note: do not use __builtins__, it is implementation detail of Python
+            # note: do not use __builtins__, it is implementation detail of cPython
             return getattr(builtins, node.id)
         if not policy.allow_globals_access and not policy.allow_locals_access:
             raise GuardRejection(
                 f"Namespace access not allowed in {context.evaluation} mode"
             )
         else:
-            raise NameError(f"{node.id} not found in locals nor globals")
+            raise NameError(f"{node.id} not found in locals, globals, nor builtins")
     if isinstance(node, ast.Attribute):
         value = eval_node(node.value, context)
         if policy.can_get_attr(value, node.attr):
@@ -540,7 +549,7 b' def eval_node(node: Union[ast.AST, None], context: EvaluationContext):'
             func,  # not joined to avoid calling `repr`
             f"not allowed in {context.evaluation} mode",
         )
-    raise ValueError("Unhandled node", node)
+    raise ValueError("Unhandled node", ast.dump(node))
 
 
 SUPPORTED_EXTERNAL_GETITEM = {
@@ -552,6 +561,7 b' SUPPORTED_EXTERNAL_GETITEM = {'
     ("numpy", "void"),
 }
 
+
 BUILTIN_GETITEM: Set[InstancesHaveGetItem] = {
     dict,
     str,
@@ -583,6 +593,8 b' set_non_mutating_methods = set(dir(set)) & set(dir(frozenset))'
 dict_keys: Type[collections.abc.KeysView] = type({}.keys())
 method_descriptor: Any = type(list.copy)
 
+NUMERICS = {int, float, complex}
+
 ALLOWED_CALLS = {
     bytes,
     *_list_methods(bytes),
@@ -600,6 +612,8 b' ALLOWED_CALLS = {'
     *_list_methods(str),
     tuple,
     *_list_methods(tuple),
+    *NUMERICS,
+    *[method for numeric_cls in NUMERICS for method in _list_methods(numeric_cls)],
     collections.deque,
     *_list_methods(collections.deque, list_non_mutating_methods),
     collections.defaultdict,
@@ -624,12 +638,13 b' BUILTIN_GETATTR: Set[MayHaveGetattr] = {'
     frozenset,
     object,
     type,  # `type` handles a lot of generic cases, e.g. numbers as in `int.real`.
+    *NUMERICS,
     dict_keys,
     method_descriptor,
 }
 
 
-BUILTIN_OPERATIONS = {int, float, complex, *BUILTIN_GETATTR}
+BUILTIN_OPERATIONS = {*BUILTIN_GETATTR}
 
 EVALUATION_POLICIES = {
     "minimal": EvaluationPolicy(

IPython/core/tests/test_guarded_eval.py

@@ -1,4 +1,5 b''
 from typing import NamedTuple
+from functools import partial
 from IPython.core.guarded_eval import (
     EvaluationContext,
     GuardRejection,
@@ -9,12 +10,19 b' from IPython.testing import decorators as dec'
 import pytest
 
 
-def limited(**kwargs):
-    return EvaluationContext(locals=kwargs, globals={}, evaluation="limited")
+def create_context(evaluation: str, **kwargs):
+    return EvaluationContext(locals=kwargs, globals={}, evaluation=evaluation)
 
 
-def unsafe(**kwargs):
-    return EvaluationContext(locals=kwargs, globals={}, evaluation="unsafe")
+forbidden = partial(create_context, "forbidden")
+minimal = partial(create_context, "minimal")
+limited = partial(create_context, "limited")
+unsafe = partial(create_context, "unsafe")
+dangerous = partial(create_context, "dangerous")
+
+LIMITED_OR_HIGHER = [limited, unsafe, dangerous]
+
+MINIMAL_OR_HIGHER = [minimal, *LIMITED_OR_HIGHER]
 
 
 @dec.skip_without("pandas")
@@ -142,7 +150,7 b' def test_set_literal():'
     assert guarded_eval('{"a"}', context) == {"a"}
 
 
-def test_if_expression():
+def test_evaluates_if_expression():
     context = limited()
     assert guarded_eval("2 if True else 3", context) == 2
     assert guarded_eval("4 if False else 5", context) == 5
@@ -178,7 +186,7 b' def test_method_descriptor():'
         [{"a": 1}, "data.keys().isdisjoint({})", "data.update()", True],
     ],
 )
-def test_calls(data, good, bad, expected):
+def test_evaluates_calls(data, good, bad, expected):
     context = limited(data=data)
     assert guarded_eval(good, context) == expected
 
@@ -194,9 +202,26 b' def test_calls(data, good, bad, expected):'
         ["list(range(20))[3:-2:3]", [3, 6, 9, 12, 15]],
     ],
 )
-def test_literals(code, expected):
-    context = limited()
-    assert guarded_eval(code, context) == expected
[e-mail]("context", LIMITED_OR_HIGHER)
+def test_evaluates_complex_cases(code, expected, context):
+    assert guarded_eval(code, context()) == expected
+
+
[e-mail](
+    "code,expected",
+    [
+        ["1", 1],
+        ["1.0", 1.0],
+        ["0xdeedbeef", 0xDEEDBEEF],
+        ["True", True],
+        ["None", None],
+        ["{}", {}],
+        ["[]", []],
+    ],
+)
[e-mail]("context", MINIMAL_OR_HIGHER)
+def test_evaluates_literals(code, expected, context):
+    assert guarded_eval(code, context()) == expected
 
 
 @pytest.mark.parametrize(
@@ -207,9 +232,9 b' def test_literals(code, expected):'
         ["~5", -6],
     ],
 )
-def test_unary_operations(code, expected):
-    context = limited()
-    assert guarded_eval(code, context) == expected
[e-mail]("context", LIMITED_OR_HIGHER)
+def test_evaluates_unary_operations(code, expected, context):
+    assert guarded_eval(code, context()) == expected
 
 
 @pytest.mark.parametrize(
@@ -228,9 +253,9 b' def test_unary_operations(code, expected):'
         ["1 & 2", 0],
     ],
 )
-def test_binary_operations(code, expected):
-    context = limited()
-    assert guarded_eval(code, context) == expected
[e-mail]("context", LIMITED_OR_HIGHER)
+def test_evaluates_binary_operations(code, expected, context):
+    assert guarded_eval(code, context()) == expected
 
 
 @pytest.mark.parametrize(
@@ -262,16 +287,152 b' def test_binary_operations(code, expected):'
         ["True is True", True],
         ["False is False", True],
         ["True is False", False],
+        ["True is not True", False],
+        ["False is not True", True],
     ],
 )
-def test_comparisons(code, expected):
-    context = limited()
-    assert guarded_eval(code, context) == expected
[e-mail]("context", LIMITED_OR_HIGHER)
+def test_evaluates_comparisons(code, expected, context):
+    assert guarded_eval(code, context()) == expected
+
+
+def test_guards_comparisons():
+    class GoodEq(int):
+        pass
+
+    class BadEq(int):
+        def __eq__(self, other):
+            assert False
+
+    context = limited(bad=BadEq(1), good=GoodEq(1))
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("bad == 1", context)
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("bad != 1", context)
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("1 == bad", context)
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("1 != bad", context)
+
+    assert guarded_eval("good == 1", context) is True
+    assert guarded_eval("good != 1", context) is False
+    assert guarded_eval("1 == good", context) is True
+    assert guarded_eval("1 != good", context) is False
+
+
+def test_guards_unary_operations():
+    class GoodOp(int):
+        pass
+
+    class BadOpInv(int):
+        def __inv__(self, other):
+            assert False
+
+    class BadOpInverse(int):
+        def __inv__(self, other):
+            assert False
+
+    context = limited(good=GoodOp(1), bad1=BadOpInv(1), bad2=BadOpInverse(1))
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("~bad1", context)
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("~bad2", context)
+
+
+def test_guards_binary_operations():
+    class GoodOp(int):
+        pass
 
+    class BadOp(int):
+        def __add__(self, other):
+            assert False
 
-def test_access_builtins():
+    context = limited(good=GoodOp(1), bad=BadOp(1))
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("1 + bad", context)
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("bad + 1", context)
+
+    assert guarded_eval("good + 1", context) == 2
+    assert guarded_eval("1 + good", context) == 2
+
+
+def test_guards_attributes():
+    class GoodAttr(float):
+        pass
+
+    class BadAttr1(float):
+        def __getattr__(self, key):
+            assert False
+
+    class BadAttr2(float):
+        def __getattribute__(self, key):
+            assert False
+
+    context = limited(good=GoodAttr(0.5), bad1=BadAttr1(0.5), bad2=BadAttr2(0.5))
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("bad1.as_integer_ratio", context)
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("bad2.as_integer_ratio", context)
+
+    assert guarded_eval("good.as_integer_ratio()", context) == (1, 2)
+
+
[e-mail]("context", MINIMAL_OR_HIGHER)
+def test_access_builtins(context):
+    assert guarded_eval("round", context()) == round
+
+
+def test_access_builtins_fails():
     context = limited()
-    assert guarded_eval("round", context) == round
+    with pytest.raises(NameError):
+        guarded_eval("this_is_not_builtin", context)
+
+
+def test_rejects_forbidden():
+    context = forbidden()
+    with pytest.raises(GuardRejection):
+        guarded_eval("1", context)
+
+
+def test_guards_locals_and_globals():
+    context = EvaluationContext(
+        locals={"local_a": "a"}, globals={"global_b": "b"}, evaluation="minimal"
+    )
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("local_a", context)
+
+    with pytest.raises(GuardRejection):
+        guarded_eval("global_b", context)
+
+
+def test_access_locals_and_globals():
+    context = EvaluationContext(
+        locals={"local_a": "a"}, globals={"global_b": "b"}, evaluation="limited"
+    )
+    assert guarded_eval("local_a", context) == "a"
+    assert guarded_eval("global_b", context) == "b"
+
+
[e-mail](
+    "code",
+    ["def func(): pass", "class C: pass", "x = 1", "x += 1", "del x", "import ast"],
+)
[e-mail]("context", [minimal(), limited(), unsafe()])
+def test_rejects_side_effect_syntax(code, context):
+    with pytest.raises(SyntaxError):
+        guarded_eval(code, context)
 
 
 def test_subscript():