From f0fd2aeb09c78386456d8da88d88cfdaad440c81 Mon Sep 17 00:00:00 2001 From: Arun Banala Date: Mon, 8 Jul 2019 15:13:42 +0100 Subject: SERVER-41829 findAndModify ignores filter expressions that are not objects (cherry picked from commit de511c6bbf22d662912f228a3ac7a8e7a8bc3c61) (cherry picked from commit 0f41fa2d430cca4912820ed0fe929e193b005e94) --- .../core/find_and_modify_invalid_query_params.js | 93 ++++++++++++++++++++++ src/mongo/db/query/find_and_modify_request.cpp | 34 +++++++- .../db/query/find_and_modify_request_test.cpp | 32 ++++++++ src/mongo/shell/crud_api.js | 6 +- 4 files changed, 159 insertions(+), 6 deletions(-) create mode 100644 jstests/core/find_and_modify_invalid_query_params.js diff --git a/jstests/core/find_and_modify_invalid_query_params.js b/jstests/core/find_and_modify_invalid_query_params.js new file mode 100644 index 00000000000..a54d9217b24 --- /dev/null +++ b/jstests/core/find_and_modify_invalid_query_params.js @@ -0,0 +1,93 @@ +/** + * Test that the 'findAndModify' command throws the expected errors for invalid query, sort and + * projection parameters. This test exercises the fix for SERVER-41829. + * @tags: [assumes_unsharded_collection] + */ +(function() { + "use strict"; + + const coll = db.find_and_modify_invalid_inputs; + coll.drop(); + coll.insert({_id: 0}); + coll.insert({_id: 1}); + + function assertFailedWithCode(cmd, errorCode) { + const err = assert.throws(() => coll.findAndModify(cmd)); + assert.eq(err.code, errorCode); + } + + function assertWorked(cmd, expectedValue) { + const out = assert.doesNotThrow(() => coll.findAndModify(cmd)); + assert.eq(out.value, expectedValue); + } + + // Verify that the findAndModify command works when we supply a valid query. + let out = coll.findAndModify({query: {_id: 1}, update: {$set: {value: "basic"}}, new: true}); + assert.eq(out, {_id: 1, value: "basic"}); + + // Verify that invalid 'query' object fails. + assertFailedWithCode({query: null, update: {value: 2}}, 31160); + assertFailedWithCode({query: 1, update: {value: 2}}, 31160); + assertFailedWithCode({query: "{_id: 1}", update: {value: 2}}, 31160); + assertFailedWithCode({query: false, update: {value: 2}}, 31160); + + // Verify that missing and empty query object is allowed. + assertWorked({update: {$set: {value: "missingQuery"}}, new: true}, "missingQuery"); + assertWorked({query: {}, update: {$set: {value: "emptyQuery"}}, new: true}, "emptyQuery"); + + // Verify that command works when we supply a valid sort specification. + assertWorked({sort: {_id: -1}, update: {$set: {value: "sort"}}, new: true}, "sort"); + + // Verify that invaid 'sort' object fails. + assertFailedWithCode({sort: null, update: {value: 2}}, 31174); + assertFailedWithCode({sort: 1, update: {value: 2}}, 31174); + assertFailedWithCode({sort: "{_id: 1}", update: {value: 2}}, 31174); + assertFailedWithCode({sort: false, update: {value: 2}}, 31174); + + // Verify that missing and empty 'sort' object is allowed. + assertWorked({update: {$set: {value: "missingSort"}}, new: true}, "missingSort"); + assertWorked({sort: {}, update: {$set: {value: "emptySort"}}, new: true}, "emptySort"); + + // Verify that the 'fields' projection works. + assertWorked({fields: {_id: 0}, update: {$set: {value: "project"}}, new: true}, "project"); + + // Verify that invaid 'fields' object fails. + assertFailedWithCode({fields: null, update: {value: 2}}, 31175); + assertFailedWithCode({fields: 1, update: {value: 2}}, 31175); + assertFailedWithCode({fields: "{_id: 1}", update: {value: 2}}, 31175); + assertFailedWithCode({fields: false, update: {value: 2}}, 31175); + + // Verify that missing and empty 'fields' object is allowed. Also verify that the command + // projects all the fields. + assertWorked({update: {$set: {value: "missingFields"}}, new: true}, "missingFields"); + assertWorked({fields: {}, update: {$set: {value: "emptyFields"}}, new: true}, "emptyFields"); + + // Verify that findOneAndDelete() shell helper throws the same errors as findAndModify(). + let err = assert.throws(() => coll.findOneAndDelete("{_id: 1}")); + assert.eq(err.code, 31160); + err = assert.throws(() => coll.findOneAndDelete(null, {sort: 1})); + assert.eq(err.code, 31174); + + // Verify that findOneAndReplace() shell helper throws the same errors as findAndModify(). + err = assert.throws(() => coll.findOneAndReplace("{_id: 1}", {})); + assert.eq(err.code, 31160); + err = assert.throws(() => coll.findOneAndReplace(null, {}, {sort: 1})); + assert.eq(err.code, 31174); + + // Verify that findOneAndUpdate() shell helper throws the same errors as findAndModify(). + err = assert.throws(() => coll.findOneAndUpdate("{_id: 1}", {$set: {value: "new"}})); + assert.eq(err.code, 31160); + err = assert.throws(() => coll.findOneAndUpdate(null, {$set: {value: "new"}}, {sort: 1})); + assert.eq(err.code, 31174); + + // Verify that find and modify shell helpers allow null query object. + out = + coll.findOneAndUpdate(null, {$set: {value: "findOneAndUpdate"}}, {returnNewDocument: true}); + assert.eq(out.value, "findOneAndUpdate"); + + out = coll.findOneAndReplace(null, {value: "findOneAndReplace"}, {returnNewDocument: true}); + assert.eq(out.value, "findOneAndReplace"); + + out = coll.findOneAndDelete(null); + assert.eq(out.value, "findOneAndReplace"); +})(); diff --git a/src/mongo/db/query/find_and_modify_request.cpp b/src/mongo/db/query/find_and_modify_request.cpp index e7ba7bab801..68c895baf21 100644 --- a/src/mongo/db/query/find_and_modify_request.cpp +++ b/src/mongo/db/query/find_and_modify_request.cpp @@ -110,10 +110,38 @@ BSONObj FindAndModifyRequest::toBSON() const { StatusWith FindAndModifyRequest::parseFromBSON(NamespaceString fullNs, const BSONObj& cmdObj) { - BSONObj query = cmdObj.getObjectField(kQueryField); - BSONObj fields = cmdObj.getObjectField(kFieldProjectionField); + BSONObj query; + BSONObj sort; + BSONObj fields; + if (auto queryElement = cmdObj[kQueryField]) { + if (queryElement.type() != Object) { + return {ErrorCodes::Error(31160), + str::stream() << "'" << kQueryField << "' parameter must be an object, found " + << queryElement.type()}; + } + query = queryElement.embeddedObject(); + } + + if (auto sortElement = cmdObj[kSortField]) { + if (sortElement.type() != Object) { + return {ErrorCodes::Error(31174), + str::stream() << "'" << kSortField << "' parameter must be an object, found " + << sortElement.type()}; + } + sort = sortElement.embeddedObject(); + } + + if (auto projectionElement = cmdObj[kFieldProjectionField]) { + if (projectionElement.type() != Object) { + return {ErrorCodes::Error(31175), + str::stream() << "'" << kFieldProjectionField + << "' parameter must be an object, found " + << projectionElement.type()}; + } + fields = projectionElement.embeddedObject(); + } + BSONObj updateObj = cmdObj.getObjectField(kUpdateField); - BSONObj sort = cmdObj.getObjectField(kSortField); BSONObj collation; { diff --git a/src/mongo/db/query/find_and_modify_request_test.cpp b/src/mongo/db/query/find_and_modify_request_test.cpp index 57b3f3bacc0..6cb827363c1 100644 --- a/src/mongo/db/query/find_and_modify_request_test.cpp +++ b/src/mongo/db/query/find_and_modify_request_test.cpp @@ -483,5 +483,37 @@ TEST(FindAndModifyRequest, ParseWithCollationTypeMismatch) { ASSERT_EQUALS(parseStatus.getStatus(), ErrorCodes::TypeMismatch); } +TEST(FindAndModifyRequest, InvalidQueryParameter) { + BSONObj cmdObj(fromjson(R"json({ + findAndModify: 'user', + query: '{ x: 1 }', + remove: true + })json")); + + auto parseStatus = FindAndModifyRequest::parseFromBSON(NamespaceString("a.b"), cmdObj); + ASSERT_EQ(31160, parseStatus.getStatus().code()); +} + +TEST(FindAndModifyRequest, InvalidSortParameter) { + BSONObj cmdObj(fromjson(R"json({ + findAndModify: 'user', + sort: 1, + remove: true + })json")); + + auto parseStatus = FindAndModifyRequest::parseFromBSON(NamespaceString("a.b"), cmdObj); + ASSERT_EQ(31174, parseStatus.getStatus().code()); +} + +TEST(FindAndModifyRequest, InvalidFieldParameter) { + BSONObj cmdObj(fromjson(R"json({ + findAndModify: 'user', + fields: null, + remove: true + })json")); + + auto parseStatus = FindAndModifyRequest::parseFromBSON(NamespaceString("a.b"), cmdObj); + ASSERT_EQ(31175, parseStatus.getStatus().code()); +} } // unnamed namespace } // namespace mongo diff --git a/src/mongo/shell/crud_api.js b/src/mongo/shell/crud_api.js index e9678cad16a..05f444b867b 100644 --- a/src/mongo/shell/crud_api.js +++ b/src/mongo/shell/crud_api.js @@ -667,7 +667,7 @@ DBCollection.prototype.updateMany = function(filter, update, options) { DBCollection.prototype.findOneAndDelete = function(filter, options) { var opts = Object.extend({}, options || {}); // Set up the command - var cmd = {query: filter, remove: true}; + var cmd = {query: filter || {}, remove: true}; if (opts.sort) { cmd.sort = opts.sort; @@ -725,7 +725,7 @@ DBCollection.prototype.findOneAndReplace = function(filter, replacement, options } // Set up the command - var cmd = {query: filter, update: replacement}; + var cmd = {query: filter || {}, update: replacement}; if (opts.sort) { cmd.sort = opts.sort; } @@ -790,7 +790,7 @@ DBCollection.prototype.findOneAndUpdate = function(filter, update, options) { } // Set up the command - var cmd = {query: filter, update: update}; + var cmd = {query: filter || {}, update: update}; if (opts.sort) { cmd.sort = opts.sort; } -- cgit v1.2.1