JS: remove the second argument of findByIdAndUpdate as a NoSQL sink

[erik-krogh]

2 things:

1. I was looking at findByIdAndUpdate, and I was surprised we flag the second argument as a NoSQL sink.
   See doc: https://mongoosejs.com/docs/tutorials/findoneandupdate.html. The second argument is the data given to the update function, so I'm unsure how that could be unsafe?

2. A friend of mine likes to use mongoose.Types.ObjectId.isValid as a sanitizer for NoSQL injection.
   The function checks if an input is a string of a certain length, so it is a nice sanitizer for object-injection in general.
   I therefore decided to put it in TaintedObject.qll, let me know if you want it somewhere else.
   The function is used a decent amount in the wild.

Evaluation looks good.
There is one lost result, where we no longer flag the second argument on a call to update.
(We still flag the first argument to that same call).

[asgerf]

I was looking at findByIdAndUpdate, and I was surprised we flag the second argument as a NoSQL sink.

I remember also being surprised at this when porting the SQL models to MaD. As far as I understand, Mongoose lets you use MongoDB update operators in updates, so they are sinks for the same reason they are sinks in MongoDB.

[erik-krogh]

I remember also being surprised at this when porting the SQL models to MaD. As far as I understand, Mongoose lets you use MongoDB update operators in updates, so they are sinks for the same reason they are sinks in MongoDB.

The documentation states: Note that update(), updateMany(), findOneAndUpdate(), etc. do not execute save() middleware. If you need save middleware and full validation, first query for the document and then save() it.

So should the second argument just be removed for those methods?

[asgerf]

The issue with save() and middlewares is not important. What matters is that these functions treat their second argument as update operators.

[github-actions]

QHelp previews:

[erik-krogh]

I see. I get it now.

I rebased away the part about removing the second argument of findByIdAndUpdate etc. as a sink.

[erik-krogh]

Sorry for the spurious Python ping. My local main was dirty, and I accidentally pushed that here.

[asgerf]

👍