Automattic / mongoose

MongoDB object modeling designed to work in an asynchronous environment.
https://mongoosejs.com
MIT License
27.01k stars 3.85k forks source link

Turn off "don't cast update pipelines by default" in favor of a more secure approach #14424

Open vkarpov15 opened 9 months ago

vkarpov15 commented 9 months ago

Prerequisites

Issue

In working on #14400, I got to thinking that the fact that update pipelines aren't casted may be risky for data integrity issues. Passing in untrusted data may lead to bypassing Mongoose casting entirely.

// If `req.body.updates` is an array, no casting, so can add arbitrary fields and incorrect types for existing fields
await User.findOneAndUpdate({ _id: req.body.id }, req.body.updates);

We should consider making update pipelines opt-in, either using a mongoose.updatePipeline() helper:

await User.findOneAndUpdate({ _id: req.body.id }, mongoose.updatePipeline([{ $set: { name: 'foo' } }]));

or with an updatePipeline option:

await User.findOneAndUpdate({ _id: req.body.id }, [{ $set: { name: 'foo' } }], { updatePipeline: true });

What do you think @hasezoey @AbdelrahmanHafez ?

AbdelrahmanHafez commented 9 months ago

I agree, I'm leaning more towards the last option where we pass updatePipeline as an option because it's less verbose.

Another possible approach would be to add a global setting mongoose.set('allowUpdatePipelines', true); that defaults to false, for those who rely heavily on update pipelines, they would already know the consequences, and they won't have to pass updatePipeline every single time they want to execute an update pipeline.

I actually like the updatePipeline option more than my suggestion because an updatePipeline option is more secure than globally enabling the option, but I'm sharing it anyway. :grin:

hasezoey commented 8 months ago

I agree that it maybe should not avoid mongoose casting by default, but should still be available as opt-in (and globally, if feasible). I also agree that the option is less verbose than a wrapper function and more in-style with what mongoose is currently doing everywhere else.

I have not read into the details yet, but is updatePipeline a good option name instead of useCasting or castPipeline or lean?