From de511c6bbf22d662912f228a3ac7a8e7a8bc3c61 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 --- ...lica_sets_multi_stmt_txn_jscore_passthrough.yml | 1 + ...ti_stmt_txn_kill_primary_jscore_passthrough.yml | 1 + ..._multi_stmt_txn_stepdown_jscore_passthrough.yml | 1 + ...mt_txn_terminate_primary_jscore_passthrough.yml | 1 + .../core/find_and_modify_invalid_query_params.js | 93 ++++++++++++++++++++++ src/mongo/db/query/find_and_modify_request.cpp | 27 ++++++- .../db/query/find_and_modify_request_test.cpp | 33 ++++++++ src/mongo/shell/crud_api.js | 6 +- 8 files changed, 157 insertions(+), 6 deletions(-) create mode 100644 jstests/core/find_and_modify_invalid_query_params.js diff --git a/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_jscore_passthrough.yml b/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_jscore_passthrough.yml index e259d666b48..3fcbee0ca78 100644 --- a/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_jscore_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_jscore_passthrough.yml @@ -29,6 +29,7 @@ selector: - jstests/core/explain_execution_error.js - jstests/core/expr.js - jstests/core/find9.js + - jstests/core/find_and_modify_invalid_query_params.js - jstests/core/find_getmore_bsonsize.js - jstests/core/find_getmore_cmd.js - jstests/core/geo_allowedcomparisons.js diff --git a/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_kill_primary_jscore_passthrough.yml b/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_kill_primary_jscore_passthrough.yml index 62a3289a1d8..23fee6aafca 100644 --- a/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_kill_primary_jscore_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_kill_primary_jscore_passthrough.yml @@ -28,6 +28,7 @@ selector: - jstests/core/explain_execution_error.js - jstests/core/expr.js - jstests/core/find9.js + - jstests/core/find_and_modify_invalid_query_params.js - jstests/core/find_getmore_bsonsize.js - jstests/core/find_getmore_cmd.js - jstests/core/geo_allowedcomparisons.js diff --git a/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_stepdown_jscore_passthrough.yml b/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_stepdown_jscore_passthrough.yml index fe9b5c962c8..abb292841f9 100644 --- a/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_stepdown_jscore_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_stepdown_jscore_passthrough.yml @@ -28,6 +28,7 @@ selector: - jstests/core/explain_execution_error.js - jstests/core/expr.js - jstests/core/find9.js + - jstests/core/find_and_modify_invalid_query_params.js - jstests/core/find_getmore_bsonsize.js - jstests/core/find_getmore_cmd.js - jstests/core/geo_allowedcomparisons.js diff --git a/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_terminate_primary_jscore_passthrough.yml b/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_terminate_primary_jscore_passthrough.yml index dced4b45c1f..c844f10647e 100644 --- a/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_terminate_primary_jscore_passthrough.yml +++ b/buildscripts/resmokeconfig/suites/replica_sets_multi_stmt_txn_terminate_primary_jscore_passthrough.yml @@ -28,6 +28,7 @@ selector: - jstests/core/explain_execution_error.js - jstests/core/expr.js - jstests/core/find9.js + - jstests/core/find_and_modify_invalid_query_params.js - jstests/core/find_getmore_bsonsize.js - jstests/core/find_getmore_cmd.js - jstests/core/geo_allowedcomparisons.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 f98e8a00eb3..20f62d2a407 100644 --- a/src/mongo/db/query/find_and_modify_request.cpp +++ b/src/mongo/db/query/find_and_modify_request.cpp @@ -168,9 +168,23 @@ StatusWith FindAndModifyRequest::parseFromBSON(NamespaceSt for (auto&& field : cmdObj.getFieldNames>()) { if (field == kQueryField) { - query = cmdObj.getObjectField(kQueryField); + 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(); } else if (field == kSortField) { - sort = cmdObj.getObjectField(kSortField); + 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(); } else if (field == kRemoveField) { isRemove = cmdObj[kRemoveField].trueValue(); } else if (field == kUpdateField) { @@ -178,7 +192,14 @@ StatusWith FindAndModifyRequest::parseFromBSON(NamespaceSt } else if (field == kNewField) { shouldReturnNew = cmdObj[kNewField].trueValue(); } else if (field == kFieldProjectionField) { - fields = cmdObj.getObjectField(kFieldProjectionField); + 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(); } else if (field == kUpsertField) { isUpsert = cmdObj[kUpsertField].trueValue(); } else if (field == kBypassDocumentValidationField) { 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 29f0fc4ac60..c60590a2a01 100644 --- a/src/mongo/db/query/find_and_modify_request_test.cpp +++ b/src/mongo/db/query/find_and_modify_request_test.cpp @@ -649,5 +649,38 @@ TEST(FindAndModifyRequest, RejectsBothArrayFiltersAndPipelineUpdate) { auto swRequestOneFilter = FindAndModifyRequest::parseFromBSON(NamespaceString("a.b"), cmdObj); ASSERT_EQ(swRequestOneFilter.getStatus(), ErrorCodes::FailedToParse); } + +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 ffed81f9c15..bcd245f4878 100644 --- a/src/mongo/shell/crud_api.js +++ b/src/mongo/shell/crud_api.js @@ -708,7 +708,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; @@ -771,7 +771,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; } @@ -839,7 +839,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