From a04069c537badeb9aec692195007e7e29b14022d Mon Sep 17 00:00:00 2001 From: "A. Jesse Jiryu Davis" Date: Sun, 21 Jul 2013 16:26:43 -0400 Subject: [PATCH] Additional input validation for $near, $nearSphere, and geoNear. --- jstests/geo_mindistance.js | 75 -------------- jstests/geo_query_input_validation.js | 107 ++++++++++++++++++++ jstests/geo_s2near.js | 2 +- jstests/geonear_cmd_input_validation.js | 126 ++++++++++++++++++++++++ src/mongo/db/geo/geoconstants.h | 24 +++++ src/mongo/db/geo/geonear.cpp | 17 +++- src/mongo/db/geo/geoquery.cpp | 44 +++++---- src/mongo/db/geo/s2common.h | 3 +- src/mongo/db/index/2d_index_cursor.cpp | 16 ++- src/mongo/db/index/s2_access_method.cpp | 7 +- 10 files changed, 312 insertions(+), 109 deletions(-) create mode 100644 jstests/geo_query_input_validation.js create mode 100644 jstests/geonear_cmd_input_validation.js create mode 100644 src/mongo/db/geo/geoconstants.h diff --git a/jstests/geo_mindistance.js b/jstests/geo_mindistance.js index d9909a5a4e984..16f76b9fa5a59 100644 --- a/jstests/geo_mindistance.js +++ b/jstests/geo_mindistance.js @@ -213,81 +213,6 @@ assert.eq( + cmdResult.results.length ); -// -// Test $minDistance input validation for $near and $nearSphere queries, -// and for geoNear command. -// - -/** Some bad inputs for $near and $nearSphere. */ -var badMinDistance, badMinDistances = [-1, undefined, 'foo']; -for (var i = 0; i < badMinDistances.length; i++) { - badMinDistance = badMinDistances[i]; - - assert.throws( - function(minDistance) { - t.find({loc: {$nearSphere: geoJSONPoint, $minDistance: minDistance}}).next(); - }, - [badMinDistance], - "$nearSphere with GeoJSON point should've failed with $minDistance = " + badMinDistance); - - assert.throws( - function(minDistance) { - t.find({loc: {$nearSphere: legacyPoint, $minDistance: minDistance}}).next(); - }, - [badMinDistance], - "$nearSphere with legacy coordinates should've failed with $minDistance = " + badMinDistance); - - assert.throws( - function(minDistance) { - t.find({loc: {$near: geoJSONPoint, $minDistance: minDistance}}).next(); - }, - [badMinDistance], - "$near with GeoJSON point should've failed with $minDistance = " + badMinDistance); - - assert.commandFailed( - db.runCommand({ - geoNear: t.getName(), - near: legacyPoint, - minDistance: badMinDistance, - spherical: true - }), - "geoNear with legacy coordinates should've failed with $minDistance = " + badMinDistance); - - assert.commandFailed( - db.runCommand({ - geoNear: t.getName(), - near: {type: 'Point', coordinates: [0, 0]}, - minDistance: badMinDistance, - spherical: true - }), - "geoNear with GeoJSON point should've failed with $minDistance = " + badMinDistance); -} - -/* Can't be more than half earth radius in meters. */ -badMinDistance = Math.PI * earthRadiusMeters + 10; -assert.throws( - function(minDistance) { - t.find({loc: {$near: geoJSONPoint, $minDistance: minDistance}}).next(); - }, - [badMinDistance], - "$near should've failed with $minDistance = " + badMinDistance); - -assert.throws( - function(minDistance) { - t.find({loc: {$nearSphere: geoJSONPoint, $minDistance: minDistance}}).next(); - }, - [badMinDistance], - "$nearSphere should've failed with $minDistance = " + badMinDistance); - -/* Can't be more than pi. */ -badMinDistance = Math.PI + 0.1; -assert.throws( - function(minDistance) { - t.find({loc: {$nearSphere: legacyPoint, $minDistance: minDistance}}).next(); - }, - [badMinDistance], - "$near should've failed with $minDistance = " + badMinDistance); - // // Verify that we throw errors using 2d index with $minDistance. // ($minDistance requires a 2dsphere index, not supported with 2d.) diff --git a/jstests/geo_query_input_validation.js b/jstests/geo_query_input_validation.js new file mode 100644 index 0000000000000..4eebe0937947d --- /dev/null +++ b/jstests/geo_query_input_validation.js @@ -0,0 +1,107 @@ +// +// Test input validation for $near and $nearSphere queries. +// +var t = db.geo_query_input_validation; + +// The test matrix. Some combinations are not supported: +// 2d index and $minDistance. +// 2dsphere index, $near, and legacy coordinates. +var indexTypes = ['2d', '2dsphere'], + queryTypes = ['$near', '$nearSphere'], + pointTypes = [ + {$geometry: {type: 'Point', coordinates: [0, 0]}}, + [0, 0]], + optionNames = ['$minDistance', '$maxDistance'], + badDistances = [-1, undefined, 'foo']; + +indexTypes.forEach(function(indexType) { + t.drop(); + t.createIndex({'loc': indexType}); + + queryTypes.forEach(function(queryType) { + pointTypes.forEach(function(pointType) { + optionNames.forEach(function(optionName) { + var isLegacy = Array.isArray(pointType), + pointDescription = (isLegacy ? "legacy coordinates" : "GeoJSON point"); + + // Like {loc: {$nearSphere: [0, 0], $maxDistance: 1}}. + var query = {}; + query[queryType] = pointType; + query[optionName] = 1; + + var locQuery = {loc: query}; + + if (indexType == '2d' && !isLegacy) { + // Currently doesn't throw errors but also doesn't work as + // expected: SERVER-10636. Stop processing this combination. + return; + } + + // Unsupported combinations should return errors. + if ( + (indexType == '2d' && optionName == '$minDistance') || + (indexType == '2dsphere' && queryType == '$near' && isLegacy) + ) { + assert.throws( + function() { + t.find(locQuery).itcount(); + }, + [], + queryType + " query with " + indexType + + " index and " + pointDescription + + " should've failed." + ); + + // Stop processing this combination in the test matrix. + return; + } + + // This is a supported combination. No error. + t.find(locQuery).itcount(); + + function makeQuery(distance) { + // Like {$nearSphere: geoJSONPoint, $minDistance: -1}. + var query = {}; + query[queryType] = pointType; + query[optionName] = distance; + return query; + } + + function doQuery(query) { + t.find({loc: query}).itcount(); + } + + // No error with $min/$maxDistance 1. + doQuery(makeQuery(1)); + + var outOfRangeDistances = []; + if (indexType == '2d' && queryType == '$near') { + // $maxDistance unlimited; no error. + doQuery(makeQuery(1e10)); + } else if (isLegacy) { + // Radians can't be more than pi. + outOfRangeDistances.push(Math.PI + 0.1); + } else { + // $min/$maxDistance is in meters, so distances greater + // than pi are ok, but not more than half earth's + // circumference in meters. + doQuery(makeQuery(Math.PI + 0.1)); + + var earthRadiusMeters = 6378.1 * 1000; + outOfRangeDistances.push(Math.PI * earthRadiusMeters + 100); + } + + // Try several bad values for $min/$maxDistance. + badDistances.concat(outOfRangeDistances).forEach(function(badDistance) { + + var msg = ( + queryType + " with " + pointDescription + + " and " + indexType + " index should've failed with " + + optionName + " " + badDistance); + + assert.throws(doQuery, [makeQuery(badDistance)], msg); + }); + }); + }); + }); +}); diff --git a/jstests/geo_s2near.js b/jstests/geo_s2near.js index b5f9b891090cd..136e821b4b89a 100644 --- a/jstests/geo_s2near.js +++ b/jstests/geo_s2near.js @@ -36,7 +36,7 @@ assert.throws(function() { return db.runCommand({geoNear : t.getName(), near: so // Do some basic near searches. res = t.find({ "geo" : { "$near" : { "$geometry" : origin, $maxDistance: 2000} } }).limit(10) -resNear = db.runCommand({geoNear : t.getName(), near: [0,0], num: 10, maxDistance: 2000, spherical: true}) +resNear = db.runCommand({geoNear : t.getName(), near: [0,0], num: 10, maxDistance: Math.PI, spherical: true}) assert.eq(res.itcount(), resNear.results.length, 10) res = t.find({ "geo" : { "$near" : { "$geometry" : origin } } }).limit(10) diff --git a/jstests/geonear_cmd_input_validation.js b/jstests/geonear_cmd_input_validation.js new file mode 100644 index 0000000000000..14e2843227019 --- /dev/null +++ b/jstests/geonear_cmd_input_validation.js @@ -0,0 +1,126 @@ +// +// Test input validation for geoNear command. +// +var t = db.geonear_cmd_input_validation; +t.drop(); +t.ensureIndex({loc: "2dsphere"}); + +// The test matrix. Some combinations are not supported: +// 2d index and minDistance. +// 2d index and GeoJSON +// 2dsphere index and spherical=false +var indexTypes = ['2d', '2dsphere'], + pointTypes = [ + {type: 'Point', coordinates: [0, 0]}, + [0, 0]], + sphericalOptions = [true, false], + optionNames = ['minDistance', 'maxDistance'], + badNumbers = [-1, undefined, 'foo']; + +indexTypes.forEach(function(indexType) { + t.drop(); + t.createIndex({'loc': indexType}); + + pointTypes.forEach(function(pointType) { + sphericalOptions.forEach(function(spherical) { + optionNames.forEach(function(optionName) { + var isLegacy = Array.isArray(pointType), + pointDescription = (isLegacy ? "legacy coordinates" : "GeoJSON point"); + + function makeCommand(distance) { + var command = { + geoNear: t.getName(), + near: pointType, + spherical: spherical + }; + command[optionName] = distance; + return command; + } + + // Unsupported combinations should return errors. + if ( + (indexType == '2d' && optionName == 'minDistance') || + (indexType == '2d' && !isLegacy) || + (indexType == '2dsphere' && !spherical) + ) { + assert.commandFailed( + db.runCommand(makeCommand(1)), + "geoNear with spherical=" + spherical + " and " + indexType + + " index and " + pointDescription + + " should've failed." + ); + + // Stop processing this combination in the test matrix. + return; + } + + // This is a supported combination. No error. + assert.commandWorked(db.runCommand({ + geoNear: t.getName(), + near: pointType, + spherical: spherical + })); + + // No error with min/maxDistance 1. + db.runCommand(makeCommand(1)); + + var outOfRangeDistances = []; + if (indexType == '2d') { + // maxDistance unlimited; no error. + db.runCommand(makeCommand(1e10)); + } else if (isLegacy) { + // Radians can't be more than pi. + outOfRangeDistances.push(Math.PI + 0.1); + } else { + // Meters can't be more than half circumference. + var earthRadiusMeters = 6378.1 * 1000; + outOfRangeDistances.push(Math.PI * earthRadiusMeters + 100); + } + + // Try several bad values for min/maxDistance. + badNumbers.concat(outOfRangeDistances).forEach(function(badDistance) { + + var msg = ( + "geoNear with spherical=" + spherical + " and " + + pointDescription + " and " + indexType + + " index should've failed with " + + optionName + " " + badDistance); + + assert.commandFailed( + db.runCommand(makeCommand(badDistance)), + msg); + }); + + // Bad values for limit / num. + ['num', 'limit'].forEach(function(limitOptionName) { + badNumbers.forEach(function(badLimit) { + + var msg = ( + "geoNear with spherical=" + spherical + " and " + + pointDescription + " and " + indexType + + " index should've failed with '" + + limitOptionName + "' " + badLimit); + + var command = makeCommand(1); + command[limitOptionName] = badLimit; + assert.commandFailed(db.runCommand(command), msg); + }); + }); + + // Bad values for distanceMultiplier. + badNumbers.forEach(function(badNumber) { + + var msg = ( + "geoNear with spherical=" + spherical + " and " + + pointDescription + " and " + indexType + + " index should've failed with distanceMultiplier " + + badNumber); + + var command = makeCommand(1); + command['distanceMultiplier'] = badNumber; + assert.commandFailed(db.runCommand(command), msg); + }); + }); + }); + }); +}); diff --git a/src/mongo/db/geo/geoconstants.h b/src/mongo/db/geo/geoconstants.h new file mode 100644 index 0000000000000..1010572492fc3 --- /dev/null +++ b/src/mongo/db/geo/geoconstants.h @@ -0,0 +1,24 @@ +/** +* Copyright (C) 2013 MongoDB Inc. +* +* This program is free software: you can redistribute it and/or modify +* it under the terms of the GNU Affero General Public License, version 3, +* as published by the Free Software Foundation. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU Affero General Public License for more details. +* +* You should have received a copy of the GNU Affero General Public License +* along with this program. If not, see . +*/ + +#pragma once + +namespace mongo { + + // Thanks, Wikipedia. + const double kRadiusOfEarthInMeters = (6378.1 * 1000); + +} // namespace mongo diff --git a/src/mongo/db/geo/geonear.cpp b/src/mongo/db/geo/geonear.cpp index 92093e5e307ab..79eb52d6c98a6 100644 --- a/src/mongo/db/geo/geonear.cpp +++ b/src/mongo/db/geo/geonear.cpp @@ -38,9 +38,13 @@ namespace mongo { GeoNearArguments::GeoNearArguments(const BSONObj &cmdObj) { - const char* limitName = cmdObj["num"].isNumber() ? "num" : "limit"; - if (cmdObj[limitName].isNumber()) { - numWanted = cmdObj[limitName].numberInt(); + // If 'num' is passed, use it and ignore 'limit'. Otherwise use 'limit'. + const char* limitName = !cmdObj["num"].eoo() ? "num" : "limit"; + BSONElement eNumWanted = cmdObj[limitName]; + if (!eNumWanted.eoo()) { + uassert(17032, str::stream() << limitName << " must be a number", + eNumWanted.isNumber()); + numWanted = eNumWanted.numberInt(); } else { numWanted = 100; } @@ -61,8 +65,11 @@ namespace mongo { query = cmdObj["query"].embeddedObject(); } - if (cmdObj["distanceMultiplier"].isNumber()) { - distanceMultiplier = cmdObj["distanceMultiplier"].number(); + BSONElement eDistanceMultiplier = cmdObj["distanceMultiplier"]; + if (!eDistanceMultiplier.eoo()) { + uassert(17033, "distanceMultiplier must be a number", eDistanceMultiplier.isNumber()); + distanceMultiplier = eDistanceMultiplier.number(); + uassert(17034, "distanceMultiplier must be non-negative", distanceMultiplier >= 0); } else { distanceMultiplier = 1.0; } diff --git a/src/mongo/db/geo/geoquery.cpp b/src/mongo/db/geo/geoquery.cpp index 85a72a0d2f6ea..916d9999aa140 100644 --- a/src/mongo/db/geo/geoquery.cpp +++ b/src/mongo/db/geo/geoquery.cpp @@ -15,6 +15,7 @@ */ #include "mongo/db/geo/geoquery.h" +#include "mongo/db/geo/geoconstants.h" namespace mongo { @@ -29,37 +30,35 @@ namespace mongo { fromRadians = (FLAT == centroid.crs); if (!obj["minDistance"].eoo()) { - if (obj["minDistance"].isNumber()) { - double distArg = obj["minDistance"].number(); - uassert(16901, "minDistance must be non-negative", distArg >= 0.0); - if (fromRadians) { - minDistance = distArg * radius; - } else { - minDistance = distArg; - } + uassert(17035, "minDistance must be a number", obj["minDistance"].isNumber()); + double distArg = obj["minDistance"].number(); + uassert(16901, "minDistance must be non-negative", distArg >= 0.0); + if (fromRadians) { + minDistance = distArg * radius; } else { - return false; + minDistance = distArg; } } if (!obj["maxDistance"].eoo()) { - if (obj["maxDistance"].isNumber()) { - double distArg = obj["maxDistance"].number(); - uassert(16902, "maxDistance must be non-negative", distArg >= 0.0); - if (fromRadians) { - maxDistance = distArg * radius; - } else { - maxDistance = distArg; - } + uassert(17036, "maxDistance must be a number", obj["maxDistance"].isNumber()); + double distArg = obj["maxDistance"].number(); + log() << "parseFromGeoNear got maxDistance " << distArg; + uassert(16902, "maxDistance must be non-negative", distArg >= 0.0); + if (fromRadians) { + maxDistance = distArg * radius; } else { - return false; + maxDistance = distArg; } + + uassert(17037, "maxDistance too large", maxDistance <= M_PI * radius); } return true; } bool NearQuery::parseFrom(const BSONObj &obj) { bool hasGeometry = false; + bool hasMaxDistance = false; // First, try legacy near, e.g.: // t.find({ loc : { $nearSphere: [0,0], $minDistance: 1, $maxDistance: 3 }}) @@ -95,9 +94,16 @@ namespace mongo { uassert(16895, "$maxDistance must be a number", e.isNumber()); maxDistance = e.Number(); uassert(16896, "$maxDistance must be non-negative", maxDistance >= 0.0); + hasMaxDistance = true; } } + double maxValidDistance = fromRadians ? M_PI : kRadiusOfEarthInMeters * M_PI; + + uassert(17038, "$minDistance too large", minDistance < maxValidDistance); + uassert(17039, "$maxDistance too large", + !hasMaxDistance || maxDistance <= maxValidDistance); + if (hasGeometry) { return true; } // Next, try "new" near: @@ -125,10 +131,12 @@ namespace mongo { uassert(16897, "$minDistance must be a number", e.isNumber()); minDistance = e.Number(); uassert(16898, "$minDistance must be non-negative", minDistance >= 0.0); + uassert(17084, "$minDistance too large", minDistance < maxValidDistance); } else if (mongoutils::str::equals(e.fieldName(), "$maxDistance")) { uassert(16899, "$maxDistance must be a number", e.isNumber()); maxDistance = e.Number(); uassert(16900, "$maxDistance must be non-negative", maxDistance >= 0.0); + uassert(16992, "$maxDistance too large", maxDistance <= maxValidDistance); } } return hasGeometry; diff --git a/src/mongo/db/geo/s2common.h b/src/mongo/db/geo/s2common.h index ea933876156ad..7a93fe248cd69 100644 --- a/src/mongo/db/geo/s2common.h +++ b/src/mongo/db/geo/s2common.h @@ -16,6 +16,7 @@ #include "mongo/db/diskloc.h" #include "mongo/db/geo/geoparser.h" +#include "mongo/db/geo/geoconstants.h" #include "third_party/s2/s2.h" #include "third_party/s2/s2regioncoverer.h" #include "third_party/s2/s2cell.h" @@ -28,8 +29,6 @@ namespace mongo { struct S2IndexingParams { - static const double kRadiusOfEarthInMeters; - // Since we take the cartesian product when we generate keys for an insert, // we need a cap. size_t maxKeysPerInsert; diff --git a/src/mongo/db/index/2d_index_cursor.cpp b/src/mongo/db/index/2d_index_cursor.cpp index 9cd88fbc333cc..3692688a2b259 100644 --- a/src/mongo/db/index/2d_index_cursor.cpp +++ b/src/mongo/db/index/2d_index_cursor.cpp @@ -1782,8 +1782,13 @@ namespace mongo { cmdObj["minDistance"].eoo()); double maxDistance = numeric_limits::max(); - if (cmdObj["maxDistance"].isNumber()) + BSONElement eMaxDistance = cmdObj["maxDistance"]; + + if (!eMaxDistance.eoo()) { + uassert(17085, "maxDistance must be a number", eMaxDistance.isNumber()); maxDistance = cmdObj["maxDistance"].number(); + uassert(17086, "maxDistance must be non-negative", maxDistance >= 0); + } GeoDistType type = parsedArgs.isSpherical ? GEO_SPHERE : GEO_PLANE; @@ -1875,7 +1880,6 @@ namespace mongo { case BSONObj::opNEAR: { BSONObj n = e.embeddedObject(); e = n.firstElement(); - twod_internal::GeoDistType type; if (strcmp(e.fieldName(), "$nearSphere") == 0) { type = twod_internal::GEO_SPHERE; @@ -1901,8 +1905,14 @@ namespace mongo { } { BSONElement e = n["$maxDistance"]; - if (e.isNumber()) + if (!e.eoo()) { + uassert(17087, "$maxDistance must be a number", e.isNumber()); maxDistance = e.numberDouble(); + uassert(16989, "$maxDistance must be non-negative", maxDistance >= 0); + if (twod_internal::GEO_SPHERE == type) { + uassert(17088, "$maxDistance too large", maxDistance <= M_PI); + } + } } bool uniqueDocs = false; diff --git a/src/mongo/db/index/s2_access_method.cpp b/src/mongo/db/index/s2_access_method.cpp index 83d49da9e8a51..c286091e9594f 100644 --- a/src/mongo/db/index/s2_access_method.cpp +++ b/src/mongo/db/index/s2_access_method.cpp @@ -20,16 +20,13 @@ #include "mongo/base/status.h" #include "mongo/db/geo/geoparser.h" +#include "mongo/db/geo/geoconstants.h" #include "mongo/db/geo/s2common.h" #include "mongo/db/index_names.h" #include "mongo/db/index/s2_index_cursor.h" #include "mongo/db/jsobj.h" namespace mongo { - - // Thanks, Wikipedia. - const double S2IndexingParams::kRadiusOfEarthInMeters = (6378.1 * 1000); - static int configValueWithDefault(IndexDescriptor *desc, const string& name, int def) { BSONElement e = desc->getInfoElement(name); if (e.isNumber()) { return e.numberInt(); } @@ -44,7 +41,7 @@ namespace mongo { // This is advisory. _params.maxCellsInCovering = 50; // Near distances are specified in meters...sometimes. - _params.radius = S2IndexingParams::kRadiusOfEarthInMeters; + _params.radius = kRadiusOfEarthInMeters; // These are not advisory. _params.finestIndexedLevel = configValueWithDefault(descriptor, "finestIndexedLevel", S2::kAvgEdge.GetClosestLevel(500.0 / _params.radius));