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

findOne method returns first document, even when the query value passed is "undefined" - can create privacy issues #14948

Open
2 tasks done
Alef5750 opened this issue Oct 10, 2024 · 3 comments
Labels
backwards-breaking enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Milestone

Comments

@Alef5750
Copy link

Alef5750 commented Oct 10, 2024

Prerequisites

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

Last performant version

8.7.0

Slowed down in version

8.7.0

Node.js version

20.17.0

🦥 Performance issue

When using the findOne() method, passing it a query value of undefined, like such:
User.findOne(undefined)
the response is the first document in the User collection.
This is unique to Mongoose, and different than db.collection.findOne(undefined) in regular MongoDB, which returns null in such a case.

The problem: privacy issues. A hacker can pass 'undefined' and retrieve first user in the database which may have sensitive information.

Steps to Reproduce

  1. Create a User collection in a mongoDB database.
  2. add a users to the database with some fields (username, password, etc.)
  3. in your server's controller, run a Mongoose method getUser:export const getUser = async (req: Request, res: Response) => { try { const user = await User.findOne(undefined); **console.log(user);** res.status(200).json(user); } catch (error) { res.status(500).json({ message: "Error fetching user", error }); } };

Expected Behavior

Notice what the console logs as "user" to be the first user in your database, (and not null)

@vkarpov15 vkarpov15 added this to the 8.7.3 milestone Oct 16, 2024
@IslandRhythms
Copy link
Collaborator

IslandRhythms commented Oct 24, 2024

I believe this is expected behavior. This is not the first time this issue has been raised. If I'm not confusing this with something else, what happens under the hood is that it takes out the undefined in the search query and so what gets sent to mongo is {} which would return one document. Changing this could have some consequences on people's environments so its something that will need to be considered.

@vkarpov15 vkarpov15 modified the milestones: 8.7.3, 8.7.4 Oct 25, 2024
vkarpov15 added a commit that referenced this issue Nov 5, 2024
@vkarpov15
Copy link
Collaborator

This is currently expected behavior, and not unique to Mongoose. The following script shows that findOne(undefined) also returns the first document for with the MongoDB Node.js driver

'use strict';

const mongoose = require('mongoose');

void async function main() {
  await mongoose.connect('mongodb://127.0.0.1:27017/mongoose_test');

  const Test = mongoose.model('Test', mongoose.Schema({ name: String }));
  await Test.findOneAndUpdate({ name: 'test' }, { name: 'test' }, { upsert: true });

  // The following all print the first document in the db
  console.log(await Test.findOne(undefined));
  console.log(await Test.findOne(null));
  console.log(await Test.collection.findOne(undefined));

  // Throws "MongoServerError: Expected field filterto be of type object"
  console.log(await Test.collection.findOne(null));
}();

The major difference is that Mongoose findOne(null) returns the first document, whereas findOne(null) throws an error in the MongoDB Node.js driver. I think we should be able to make findOne(null) in Mongoose behave like findOne(null) in the MongoDB driver. undefined may be a little trickier, we will look into that

@vkarpov15 vkarpov15 modified the milestones: 8.8.1, 8.9 Nov 5, 2024
vkarpov15 added a commit that referenced this issue Nov 8, 2024
fix(model+query): make `findOne(null)`, `find(null)`, etc. throw an error instead of returning first doc
@vkarpov15 vkarpov15 modified the milestones: 8.9, 9.0 Nov 8, 2024
@vkarpov15
Copy link
Collaborator

We will ship the findOne(null) changes in 9.0 because this may be a breaking change. We will also consider making findOne(undefined) throw an error.

@vkarpov15 vkarpov15 added enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature backwards-breaking and removed performance labels Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards-breaking enhancement This issue is a user-facing general improvement that doesn't fix a bug or add a new feature
Projects
None yet
Development

No branches or pull requests

3 participants