Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 tasks done
vkarpov15 opened this issue Mar 7, 2024 · 2 comments
Open
2 tasks done
Labels
backwards-breaking discussion If you have any thoughts or comments on this issue, please share them!
Milestone

Comments

@vkarpov15
Copy link
Collaborator

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

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 ?

@vkarpov15 vkarpov15 added discussion If you have any thoughts or comments on this issue, please share them! backwards-breaking labels Mar 7, 2024
@vkarpov15 vkarpov15 added this to the 9.0 milestone Mar 7, 2024
@AbdelrahmanHafez
Copy link
Collaborator

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. 😁

@hasezoey
Copy link
Collaborator

hasezoey commented Mar 8, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-breaking discussion If you have any thoughts or comments on this issue, please share them!
Projects
None yet
Development

No branches or pull requests

3 participants