diff --git a/python/ql/src/Expressions/ContainsNonContainer.ql b/python/ql/src/Expressions/ContainsNonContainer.ql index ad0a80bb1dd..cba1a945ee2 100644 --- a/python/ql/src/Expressions/ContainsNonContainer.ql +++ b/python/ql/src/Expressions/ContainsNonContainer.ql @@ -14,6 +14,7 @@ import python import semmle.python.dataflow.new.DataFlow private import semmle.python.dataflow.new.internal.DataFlowDispatch +private import semmle.python.ApiGraphs predicate rhs_in_expr(Expr rhs, Compare cmp) { exists(Cmpop op, int i | cmp.getOp(i) = op and cmp.getComparator(i) = rhs | @@ -42,6 +43,35 @@ predicate hasDynamicMethods(Class cls) { ) } +/** + * Holds if `cls_arg` references a known container builtin type, either directly + * or as an element of a tuple. + */ +private predicate isContainerTypeArg(DataFlow::Node cls_arg) { + cls_arg = + API::builtin(["list", "tuple", "set", "frozenset", "dict", "str", "bytes", "bytearray"]) + .getAValueReachableFromSource() + or + isContainerTypeArg(DataFlow::exprNode(cls_arg.asExpr().(Tuple).getAnElt())) +} + +/** + * Holds if `rhs` is guarded by an `isinstance` check that tests for + * a container type. + */ +predicate guardedByIsinstanceContainer(DataFlow::Node rhs) { + exists( + DataFlow::GuardNode guard, DataFlow::CallCfgNode isinstance_call, DataFlow::LocalSourceNode src + | + isinstance_call = API::builtin("isinstance").getACall() and + src.flowsTo(isinstance_call.getArg(0)) and + src.flowsTo(rhs) and + isContainerTypeArg(isinstance_call.getArg(1)) and + guard = isinstance_call.asCfgNode() and + guard.controlsBlock(rhs.asCfgNode().getBasicBlock(), true) + ) +} + from Compare cmp, DataFlow::LocalSourceNode origin, DataFlow::Node rhs, Class cls where origin = classInstanceTracker(cls) and @@ -50,6 +80,7 @@ where not DuckTyping::hasUnresolvedBase(getADirectSuperclass*(cls)) and not isDecoratorApplication(origin) and not hasDynamicMethods(cls) and + not guardedByIsinstanceContainer(rhs) and rhs_in_expr(rhs.asExpr(), cmp) select cmp, "This test may raise an Exception as the $@ may be of non-container class $@.", origin, "target", cls, cls.getName() diff --git a/python/ql/test/query-tests/Expressions/general/expressions_test.py b/python/ql/test/query-tests/Expressions/general/expressions_test.py index cf4c04e0a34..f9d522d328b 100644 --- a/python/ql/test/query-tests/Expressions/general/expressions_test.py +++ b/python/ql/test/query-tests/Expressions/general/expressions_test.py @@ -317,3 +317,24 @@ def test_dynamic_methods(): # OK: __contains__ is added dynamically via setattr. if "name" in proxy: pass + +# isinstance guard should suppress non-container warning +def guarded_contains(x): + obj = XIter() + if isinstance(obj, dict): + if x in obj: # OK: guarded by isinstance + pass + +def guarded_contains_tuple(x): + obj = XIter() + if isinstance(obj, (list, dict, set)): + if x in obj: # OK: guarded by isinstance with tuple of types + pass + +# Negated isinstance guard: early return when NOT a container +def guarded_contains_negated(x): + obj = XIter() + if not isinstance(obj, dict): + return + if x in obj: # OK: guarded by negated isinstance + early return + pass