Skip to content

Commit

Permalink
Merge pull request #14691 from Automattic/vkarpov15/gh-14333
Browse files Browse the repository at this point in the history
feat(query): make sanitizeProjection prevent projecting in paths deselected in the schema
  • Loading branch information
vkarpov15 authored Jul 3, 2024
2 parents 1a0cda7 + 0904a18 commit f48df23
Show file tree
Hide file tree
Showing 5 changed files with 131 additions and 3 deletions.
65 changes: 64 additions & 1 deletion lib/query.js
Original file line number Diff line number Diff line change
Expand Up @@ -1142,6 +1142,59 @@ Query.prototype.select = function select() {
throw new TypeError('Invalid select() argument. Must be string or object.');
};

/**
* Sets this query's `sanitizeProjection` option. If set, `sanitizeProjection` does
* two things:
*
* 1. Enforces that projection values are numbers, not strings.
* 2. Prevents using `+` syntax to override properties that are deselected by default.
*
* With `sanitizeProjection()`, you can pass potentially untrusted user data to `.select()`.
*
* #### Example
*
* const userSchema = new Schema({
* name: String,
* password: { type: String, select: false }
* });
* const UserModel = mongoose.model('User', userSchema);
* const { _id } = await UserModel.create({ name: 'John', password: 'secret' })
*
* // The MongoDB server has special handling for string values that start with '$'
* // in projections, which can lead to unexpected leaking of sensitive data.
* let doc = await UserModel.findOne().select({ name: '$password' });
* doc.name; // 'secret'
* doc.password; // undefined
*
* // With `sanitizeProjection`, Mongoose forces all projection values to be numbers
* doc = await UserModel.findOne().sanitizeProjection(true).select({ name: '$password' });
* doc.name; // 'John'
* doc.password; // undefined
*
* // By default, Mongoose supports projecting in `password` using `+password`
* doc = await UserModel.findOne().select('+password');
* doc.password; // 'secret'
*
* // With `sanitizeProjection`, Mongoose prevents projecting in `password` and other
* // fields that have `select: false` in the schema.
* doc = await UserModel.findOne().sanitizeProjection(true).select('+password');
* doc.password; // undefined
*
* @method sanitizeProjection
* @memberOf Query
* @instance
* @param {Boolean} value
* @return {Query} this
* @see sanitizeProjection https://thecodebarbarian.com/whats-new-in-mongoose-5-13-sanitizeprojection.html
* @api public
*/

Query.prototype.sanitizeProjection = function sanitizeProjection(value) {
this._mongooseOptions.sanitizeProjection = value;

return this;
};

/**
* Determines the MongoDB nodes from which to read.
*
Expand Down Expand Up @@ -4872,7 +4925,17 @@ Query.prototype._applyPaths = function applyPaths() {
return;
}
this._fields = this._fields || {};
helpers.applyPaths(this._fields, this.model.schema);

let sanitizeProjection = undefined;
if (this.model != null && utils.hasUserDefinedProperty(this.model.db.options, 'sanitizeProjection')) {
sanitizeProjection = this.model.db.options.sanitizeProjection;
} else if (this.model != null && utils.hasUserDefinedProperty(this.model.base.options, 'sanitizeProjection')) {
sanitizeProjection = this.model.base.options.sanitizeProjection;
} else {
sanitizeProjection = this._mongooseOptions.sanitizeProjection;
}

helpers.applyPaths(this._fields, this.model.schema, sanitizeProjection);

let _selectPopulatedPaths = true;

Expand Down
10 changes: 8 additions & 2 deletions lib/queryHelpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ exports.createModelAndInit = function createModelAndInit(model, doc, fields, use
* ignore
*/

exports.applyPaths = function applyPaths(fields, schema) {
exports.applyPaths = function applyPaths(fields, schema, sanitizeProjection) {
// determine if query is selecting or excluding fields
let exclude;
let keys;
Expand Down Expand Up @@ -321,6 +321,10 @@ exports.applyPaths = function applyPaths(fields, schema) {

// User overwriting default exclusion
if (type.selected === false && fields[path]) {
if (sanitizeProjection) {
fields[path] = 0;
}

return;
}

Expand All @@ -345,8 +349,10 @@ exports.applyPaths = function applyPaths(fields, schema) {

// if there are other fields being included, add this one
// if no other included fields, leave this out (implied inclusion)
if (exclude === false && keys.length > 1 && !~keys.indexOf(path)) {
if (exclude === false && keys.length > 1 && !~keys.indexOf(path) && !sanitizeProjection) {
fields[path] = 1;
} else if (exclude == null && sanitizeProjection && type.selected === false) {
fields[path] = 0;
}

return;
Expand Down
41 changes: 41 additions & 0 deletions test/query.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3449,6 +3449,47 @@ describe('Query', function() {
assert.deepEqual(q._fields, { email: 1 });
});

it('sanitizeProjection option with plus paths (gh-14333) (gh-10243)', async function() {
const MySchema = Schema({
name: String,
email: String,
password: { type: String, select: false }
});
const Test = db.model('Test', MySchema);

await Test.create({ name: 'test', password: 'secret' });

let q = Test.findOne().select('+password');
let doc = await q;
assert.deepEqual(q._fields, {});
assert.strictEqual(doc.password, 'secret');

q = Test.findOne().setOptions({ sanitizeProjection: true }).select('+password');
doc = await q;
assert.deepEqual(q._fields, { password: 0 });
assert.strictEqual(doc.password, undefined);

q = Test.find().select('+password').setOptions({ sanitizeProjection: true });
doc = await q;
assert.deepEqual(q._fields, { password: 0 });
assert.strictEqual(doc.password, undefined);

q = Test.find().select('name +password').setOptions({ sanitizeProjection: true });
doc = await q;
assert.deepEqual(q._fields, { name: 1 });
assert.strictEqual(doc.password, undefined);

q = Test.find().select('+name').setOptions({ sanitizeProjection: true });
doc = await q;
assert.deepEqual(q._fields, { password: 0 });
assert.strictEqual(doc.password, undefined);

q = Test.find().select('password').setOptions({ sanitizeProjection: true });
doc = await q;
assert.deepEqual(q._fields, { password: 0 });
assert.strictEqual(doc.password, undefined);
});

it('sanitizeFilter option (gh-3944)', function() {
const MySchema = Schema({ username: String, pwd: String });
const Test = db.model('Test', MySchema);
Expand Down
13 changes: 13 additions & 0 deletions test/schema.select.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,19 @@ describe('schema select option', function() {
assert.equal(d.id, doc.id);
});

it('works if only one plus path and only one deselected field', async function() {
const MySchema = Schema({
name: String,
email: String,
password: { type: String, select: false }
});
const Test = db.model('Test', MySchema);
const { _id } = await Test.create({ name: 'test', password: 'secret' });

const doc = await Test.findById(_id).select('+password');
assert.strictEqual(doc.password, 'secret');
});

it('works with query.slice (gh-1370)', async function() {
const M = db.model('Test', new Schema({ many: { type: [String], select: false } }));

Expand Down
5 changes: 5 additions & 0 deletions types/query.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,11 @@ declare module 'mongoose' {
options?: QueryOptions<DocType> | null
): QueryWithHelpers<any, DocType, THelpers, RawDocType, 'replaceOne', TInstanceMethods>;

/**
* Sets this query's `sanitizeProjection` option. With `sanitizeProjection()`, you can pass potentially untrusted user data to `.select()`.
*/
sanitizeProjection(value: boolean): this;

/** Specifies which document fields to include or exclude (also known as the query "projection") */
select<RawDocTypeOverride extends { [P in keyof RawDocType]?: any } = {}>(
arg: string | string[] | Record<string, number | boolean | string | object>
Expand Down

0 comments on commit f48df23

Please sign in to comment.