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

[Feature] Add option to disable filters on each method #14657

Closed
2 tasks done
yhojann-cl opened this issue Jun 10, 2024 · 6 comments
Closed
2 tasks done

[Feature] Add option to disable filters on each method #14657

yhojann-cl opened this issue Jun 10, 2024 · 6 comments
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature new feature This change adds new functionality, like a new method or class
Milestone

Comments

@yhojann-cl
Copy link

yhojann-cl commented Jun 10, 2024

Prerequisites

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

🚀 Feature Proposal

The problem is that every time you work with the express framework or any other for rest services you need two types of redundant input validations, the one that belongs to the controller and the one that belongs to the data layer.

For example, I have a collection called users which has a field called name, in mongoose I can define the data type as String and a regular expression to validate its content, but from the controller, every time a user wants to search for a list of users through their name I must use a second validation (for example with jsonschema) which validates that the name property is of type string since if it is passed directly to the findOne function of mongoose it could cause a vulnerability of type NoSQL injection and bring unexpected information.

The same thing happens in other types of selections such as accessing the user account or searching for a document through its id, the end user could send a conditional object instead of plain text with the bsonId string, especially when these Data travels through the body of the post or patch request.

In summary, for each project, redundant validations must be created for almost each mongoose function to avoid unexpected executions in the functions to be executed in mongodb and this significantly increases the amount of code and complicates the development architecture, forcing the creation of additional layers of validation or custom methods that duplicate the native functions for each model to be created.

To avoid this problem, i propose something very simple, create an option to temporarily disable the use of filters in the objects that are sent as arguments in the mongoose functions, this would allow the user's input to be used in the development of the application. direct and unfiltered in a completely safe way when performing simple searches.

If the schema says that a field is of type String, then it will automatically reject any data type other than String if filters are disabled.

References:

Motivation

Improve security and save redundant code.

Example

const schema = new mongoose.Schema({
    name: {
        type: String,
        required: true,
        validate: val => !!val.match(/^[a-zA-ZáéíóúñÁÉÍÓÚÑ\s,'"`]{1,32}$/) // Why not pattern attribute?
    }
});
const model = mongoose.model('users');

const router = express.Router();
router.get(`/users`, async (req, res, next) => {
    const documents = await model
        .find({ name: $regex: req.query.search })
        .sort({ name: 'asc' })
        .options({ filters: false }) // <- Proposal here!
        .exec();
    res.status(200).json(documents);
    next();
});

This would help improve the security of everything that is sent to mongoose and reduce the number of vulnerable projects, it would also save a lot of code and work time by avoiding having to create additional validations, especially when there is more than one attribute to control where not all They are of type String.

@yhojann-cl yhojann-cl added enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature new feature This change adds new functionality, like a new method or class labels Jun 10, 2024
@vkarpov15
Copy link
Collaborator

What you're describing sounds like the sanitizeFilter option we introduced in Mongoose 6, see docs and intro blog post.

    // The following will throw an error if `req.query.search` is `{ $regex: 'abc' }` or `{ $lt: 'test' }` or any other query filter
    const documents = await model
        .find({ name: req.query.search })
        .sort({ name: 'asc' })
        .options({ sanitizeFilter: true })
        .exec();

Does the sanitizeFilter option sound like it fixes your issue?

@vkarpov15 vkarpov15 added the help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary label Jun 11, 2024
@yhojann-cl
Copy link
Author

But apparently it does not strictly sanitize according to the property type and it is still possible to send data types that do not correspond to the schema, for example:

console.log(mongoose.version); // 7.0.3
await mongoose.connect('mongodb://localhost:27017/mongoose_test');
const schema = new mongoose.Schema({ name: String });
const TestModel = mongoose.model('Test', schema);

const doc = new TestModel({ name: 'foobar' });
await doc.save();

const docs = await TestModel.find({ name: [ 'foobar' ] }).setOptions({ sanitizeFilter: true }).exec();
console.log(docs);

The results is:

[ { name: 'foobar',
    _id: new ObjectId("666d04f039e8d300b31d303f"),
    __v: 0 } ]

In the name property can send an array instead of a string value with disabled filters in options.

What I propose is an option for the input data to adhere to the schema and not just a simple elimination of filters that begin with the $ character. Or could it be a bug in that option?.

@vkarpov15 vkarpov15 modified the milestones: 8.4.5, 8.6 Jun 27, 2024
Copy link

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Jul 17, 2024
Copy link

This issue was closed because it has been inactive for 19 days and has been marked as stale.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 22, 2024
@vkarpov15 vkarpov15 removed this from the 8.6 milestone Aug 28, 2024
@vkarpov15 vkarpov15 reopened this Aug 28, 2024
@vkarpov15 vkarpov15 removed help This issue can likely be resolved in GitHub issues. No bug fixes, features, or docs necessary Stale labels Aug 28, 2024
@vkarpov15 vkarpov15 added this to the 8.7 milestone Aug 28, 2024
@vkarpov15 vkarpov15 modified the milestones: 8.7, 8.8 Sep 16, 2024
@vkarpov15
Copy link
Collaborator

#14985 adds a fix for the await TestModel.find({ name: [ 'foobar' ] }).sanitizeFilter() issue you mentioned.

If you want to disable casting for all strings (both in queries and when creating new documents), you can use mongoose.Schema.Types.String.set('cast', false);. For example, the following script throws a CastError even though Mongoose normally would convert the given Date to a string

import mongoose from 'mongoose';
import assert from 'assert';

mongoose.set('debug', true);
mongoose.Schema.Types.String.set('cast', false);

await mongoose.connect('mongodb://localhost:27017/mongoose_test');
const schema = new mongoose.Schema({
  name: { 
    type: String,
  }
});
const TestModel = mongoose.model('Test', schema);
await TestModel.deleteMany({});

const doc = new TestModel({ name: 'foobar' });
await doc.save();

// Throws a CastError
const docs2 = await TestModel.find({ name: new Date() }).setOptions({ sanitizeFilter: true }).exec();

We will consider adding an option to disable casting on a particular query, like await TestModel.find({ name: new Date() }).setOptions({ sanitizeFilter: true, cast: false }). But given that you seem to want to disable query casting on all queries, I think mongoose.Schema.Types.String.set('cast', false); is the way to go for your use case.

vkarpov15 added a commit that referenced this issue Oct 28, 2024
fix(query): make sanitizeFilter disable implicit $in
@vkarpov15
Copy link
Collaborator

We're going to close this issue, because we think that sanitizeFilters() with the fix in #14985 and cast: false + mongoose.Schema.Types.String.set('cast', false) is sufficient to address OP's use case of wanting to disable coercion of values in query filters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature new feature This change adds new functionality, like a new method or class
Projects
None yet
Development

No branches or pull requests

2 participants