Open max-schaefer opened 2 years ago
@asgerf suggested that this behaviour might be due to over-eager pruning of barrier guards: the call to action
is only safe for the purposes of this query if it is both guarded by a hasOwnProperty
check and a typeof ... === "function"'
check. We prune barrier guards by a coarse forward scan, and by restricting our set of sources the former guard is pruned away.
This sounds plausible, since if I swap lines 14 and 15 I get consistent behaviour with both sets of sources. (I think it's the wrong behaviour, since it flags this as an alert, but at least it's consistent.) That would make this a soundness bug in the data flow analysis, which isn't too concerning (to me at least).
I have the following test file for the
UnvalidatedDynamicMethodCall
query:Running the query on it (using CodeQL 2.7.6) does not flag an alert.
Now I change the
UnvalidatedDynamicMethCallQuery
library by adding the following conjunct in itsisSource
predicate:And suddenly I get an alert on the call to
action
.Quite apart from the question of whether or not this alert is correct, I don't see how adding a conjunct to the
isSource
predicate, thereby making it smaller (in this particular case, one source instead of three), can lead to more alerts being reported.