SERVER-41829 findAndModify ignores filter expressions that are not objects

8 files changed, 157 insertions, 6 deletions

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> FindAndModifyRequest::parseFromBSON(NamespaceSt
 
     for (auto&& field : cmdObj.getFieldNames<std::set<std::string>>()) {
         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> 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;
     }