From 6250931515354a158dbda57ea082d65aec8a308b 2022-12-04 00:25:52 From: krassowski <5832902+krassowski@users.noreply.github.com> Date: 2022-12-04 00:25:52 Subject: [PATCH] Increase coverage of `guard_eval` --- diff --git a/IPython/core/completer.py b/IPython/core/completer.py index 7dd585b..f2853d3 100644 --- a/IPython/core/completer.py +++ b/IPython/core/completer.py @@ -996,7 +996,8 @@ 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: diff --git a/IPython/core/guarded_eval.py b/IPython/core/guarded_eval.py index 215a36f..f71a73b 100644 --- a/IPython/core/guarded_eval.py +++ b/IPython/core/guarded_eval.py @@ -108,6 +108,7 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ class SelectivePolicy(EvaluationPolicy): class _DummyNamedTuple(NamedTuple): - pass + """Used internally to retrieve methods of named tuple instance.""" class EvaluationContext(NamedTuple): @@ -451,12 +457,15 @@ 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 @@ 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 @@ 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 @@ 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 @@ 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 @@ SUPPORTED_EXTERNAL_GETITEM = { ("numpy", "void"), } + BUILTIN_GETITEM: Set[InstancesHaveGetItem] = { dict, str, @@ -583,6 +593,8 @@ 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 @@ 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 @@ 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( diff --git a/IPython/core/tests/test_guarded_eval.py b/IPython/core/tests/test_guarded_eval.py index 94f5829..8d3495a 100644 --- a/IPython/core/tests/test_guarded_eval.py +++ b/IPython/core/tests/test_guarded_eval.py @@ -1,4 +1,5 @@ from typing import NamedTuple +from functools import partial from IPython.core.guarded_eval import ( EvaluationContext, GuardRejection, @@ -9,12 +10,19 @@ 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 @@ 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 @@ 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 @@ 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 +@pytest.mark.parametrize("context", LIMITED_OR_HIGHER) +def test_evaluates_complex_cases(code, expected, context): + assert guarded_eval(code, context()) == expected + + +@pytest.mark.parametrize( + "code,expected", + [ + ["1", 1], + ["1.0", 1.0], + ["0xdeedbeef", 0xDEEDBEEF], + ["True", True], + ["None", None], + ["{}", {}], + ["[]", []], + ], +) +@pytest.mark.parametrize("context", MINIMAL_OR_HIGHER) +def test_evaluates_literals(code, expected, context): + assert guarded_eval(code, context()) == expected @pytest.mark.parametrize( @@ -207,9 +232,9 @@ def test_literals(code, expected): ["~5", -6], ], ) -def test_unary_operations(code, expected): - context = limited() - assert guarded_eval(code, context) == expected +@pytest.mark.parametrize("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 @@ def test_unary_operations(code, expected): ["1 & 2", 0], ], ) -def test_binary_operations(code, expected): - context = limited() - assert guarded_eval(code, context) == expected +@pytest.mark.parametrize("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 @@ 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 +@pytest.mark.parametrize("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) + + +@pytest.mark.parametrize("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" + + +@pytest.mark.parametrize( + "code", + ["def func(): pass", "class C: pass", "x = 1", "x += 1", "del x", "import ast"], +) +@pytest.mark.parametrize("context", [minimal(), limited(), unsafe()]) +def test_rejects_side_effect_syntax(code, context): + with pytest.raises(SyntaxError): + guarded_eval(code, context) def test_subscript():