I don't quite know what to think about this...

I wasn't quite sure if either the old or the new these solutions could handle when the dictionary is not constructed inside the argument to the call, such as:

search = {'$where': 'this.author === "'+author+'"'}
post = posts.find_one(search) # $ result=BAD

But I tried it out locally, and it did in fact work. I guess they would since the .getParameter(0) takes us into "def node" domain, and we do back-tracking.

So basically you seem to be creating an additional taint-step specifically for NoSQL injection directly from the element of a dictionary with key $where to the argument of a mongo collection method, if API-graphs can track the dictionary flowing to that argument?

I think that works great for simple examples, but if there is any interprocedural steps, it will not be so nice. So I think we need to track these things with data-flow, so it can be part of the path shown to end-users. We might be able to achieve that with a string->dict taint-step directly when constructing a dictionary with a $where key?

You could apply the same logic to the $function and body stuff, so there would be string->dict taint-step when constructing dictionary with body key, and dict->dict taint-step when constructing dict with $function key. That would allow for all of the following examples to give alerts (I think only the first would today)

decoded = json.loads(user_controlled)

search = {
    "body": decoded,
    "args": [ "$author" ],
    "lang": "js"
}
posts.find_one({'$expr': {'$function': search}}) # $ result=BAD

posts.find_one({'$expr': {'$function': decoded}}) # $ result=BAD
posts.find_one({'$expr': decoded}) # $ result=BAD
posts.find_one(decoded) # $ result=BAD

Adding query specific taint-steps that mutate flow-state doesn't play very nicely with using Concepts, so I understand the temptation to just reuse the decoding Concepts, but I would argue that in fact no Decoding is taking place here. What I've done previously is to add query specific logic to the library modeling file, which is certainly something we could also do here (example from flask, originally added in #6989)

a string->dict taint-step directly when constructing a dictionary with a $where key

This is basically what we are doing. We just require that the dictionary also flows to a harmful place.
My intuition is that type tracking should basically be an over-approximation to data flow, so this extra requirements is more like a cheap filter.

Anyway, I added the above tests, and they all pass out of the box...

Anyway, I added the above tests, and they all pass out of the box...

Nice 👍 I guess that comes from the fact that we say after a json.loads, it could be a dict, and our default taint-steps propagate taint through dictionary literals right now.

I still think we could end up with a very long jumpstep due to the way it's written right now, which I don't think is a good solution.

Ah, that is a reasonable objection (especially now that we got better path explanations in general). I made the step target the created dictionary instead of the argument that the dictionary flows to. Dataflow is able to link the two for us.