diff --git a/api/parts/mgmt/users.js b/api/parts/mgmt/users.js index 97b1747219a..1add3567b83 100644 --- a/api/parts/mgmt/users.js +++ b/api/parts/mgmt/users.js @@ -1119,6 +1119,12 @@ usersApi.fetchNotes = async function(params) { } if (params.qstring.note_type) { + // Matched as a plain value, so it has to be a scalar. A JSON request + // body can put an object here, which Mongo would read as a query + // expression and which would widen the match instead of narrowing it. + if (!common.isQueryScalar(params.qstring.note_type)) { + return common.returnMessage(params, 400, 'Invalid parameter: note_type'); + } query.noteType = params.qstring.note_type; } diff --git a/api/utils/common.js b/api/utils/common.js index 2c3bba48358..c6eaa805874 100644 --- a/api/utils/common.js +++ b/api/utils/common.js @@ -2832,6 +2832,35 @@ common.parseUserQuery = function(raw) { return { query: query }; }; +/** + * Check that a request parameter is a scalar, so it can be used as a VALUE + * inside a MongoDB query/match document. + * + * params.qstring values are not guaranteed to be strings: a JSON request body + * puts nested objects straight into params.qstring, so a parameter an endpoint + * reads as a plain id or name can arrive as an object. In a value position + * Mongo reads such an object as a query expression rather than a value, so + * `{"view": {"$ne": null}}` turns an equality match on one document into a + * match on every document. Besides returning data the endpoint never meant to + * return, that removes the bound on how much data the query has to scan, which + * on an aggregation over the drill collections is expensive. + * + * Use this on scalar parameters (ids, names, types) before they reach a query, + * and reject the request when it returns false. It is NOT for parameters that + * are queries in their own right: validate those with common.parseUserQuery or + * common.findUnsafeMongoOperator instead. + * + * null and undefined count as scalars. Whether an absent parameter belongs in + * the query at all is the caller's decision, and the existing truthiness + * checks at the call sites already make it. + * + * @param {*} value - request parameter value + * @returns {boolean} true when the value is safe to use as a query value + */ +common.isQueryScalar = function(value) { + return value === null || typeof value !== "object"; +}; + /** * Build a short, log-safe label identifying the request's endpoint, for use in * log messages (e.g. query-rejection logs). Returns the request path plus the diff --git a/api/utils/requestProcessor.js b/api/utils/requestProcessor.js index d76fe573e01..1380b7169a4 100644 --- a/api/utils/requestProcessor.js +++ b/api/utils/requestProcessor.js @@ -2616,6 +2616,15 @@ const processRequest = (params) => { case 'delete': validateUser(() => { if (params.qstring.tokenid) { + //the id is matched as a plain value, so it has to be + //a scalar. A JSON request body can put an object + //here, which Mongo would read as a query expression, + //turning the removal of one token into the removal of + //every token the caller owns + if (!common.isQueryScalar(params.qstring.tokenid)) { + common.returnMessage(params, 400, "Invalid parameter: tokenid"); + return; + } common.db.collection("auth_tokens").remove({ "_id": params.qstring.tokenid, "owner": params.member._id + "" diff --git a/plugins/crashes/api/api.js b/plugins/crashes/api/api.js index e521bddc6bd..9265de82321 100644 --- a/plugins/crashes/api/api.js +++ b/plugins/crashes/api/api.js @@ -1137,6 +1137,14 @@ plugins.setConfigs("crashes", { else if (obParams.qstring.method === 'user_crashes') { validateRead(obParams, FEATURE_NAME, function(params) { if (params.qstring.uid) { + //uid is matched as a plain value, so it has to be a scalar. + //A JSON request body can put an object here, which Mongo + //would read as a query expression and which would widen the + //match instead of narrowing it to the one user + if (!common.isQueryScalar(params.qstring.uid)) { + common.returnMessage(params, 400, 'Invalid parameter: uid'); + return true; + } var columns = ["group", "reports", "last"]; var query = {group: {$ne: 0}, uid: params.qstring.uid}; var cursor = common.db.collection('app_crashusers' + params.app_id).find(query || {}, {_id: 0}); diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index 4ca7d12cc21..cf829cecb3b 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -1255,6 +1255,18 @@ function uploadFile(myfile, id, callback) { var limit = parseInt(params.qstring.iDisplayLength || 0); var colNames = ['rating', 'comment', 'email', 'ts']; + //these are matched as plain values in the query below, so they have to + //be scalars. A JSON request body can put an object here, which Mongo + //would read as a query expression and which would widen the match + //instead of narrowing it + var scalarParams = ['widget_id', 'version', 'platform', 'uid']; + for (var s = 0; s < scalarParams.length; s++) { + if (!common.isQueryScalar(params.qstring[scalarParams[s]])) { + common.returnMessage(params, 400, 'Invalid parameter: ' + scalarParams[s]); + return true; + } + } + if (params.qstring.widget_id) { query.widget_id = params.qstring.widget_id; } @@ -1414,6 +1426,12 @@ function uploadFile(myfile, id, callback) { var collectionName = 'feedback_widgets'; var query = {type: "rating"}; if (params.qstring.is_active) { + //matched as a plain value, so an object here would be read by + //Mongo as a query expression and would widen the match + if (!common.isQueryScalar(params.qstring.is_active)) { + common.returnMessage(params, 400, 'Invalid parameter: is_active'); + return true; + } query.is_active = params.qstring.is_active; } diff --git a/plugins/systemlogs/api/api.js b/plugins/systemlogs/api/api.js index d038f921c79..c0db88151ea 100644 --- a/plugins/systemlogs/api/api.js +++ b/plugins/systemlogs/api/api.js @@ -460,7 +460,10 @@ plugins.setConfigs("systemlogs", { else if (user.email) { query.email = user.email; } - else if (params.qstring.api_key) { + else if (params.qstring.api_key && common.isQueryScalar(params.qstring.api_key)) { + //matched as a plain value, so a non-scalar is left out of the + //query rather than handed to Mongo as a query expression, which + //would resolve the log entry to an arbitrary member query.api_key = params.qstring.api_key; } if (Object.keys(query).length) { diff --git a/plugins/views/api/api.js b/plugins/views/api/api.js index 0e3b7863343..3b70516a3cf 100644 --- a/plugins/views/api/api.js +++ b/plugins/views/api/api.js @@ -1360,6 +1360,20 @@ const escapedViewSegments = { "name": true, "segment": true, "height": true, "wi console.log('Parse device failed: ', params.qstring.device); } + //view, actionType and segment are matched as plain values against the + //drill collections below, so they have to be scalars. A JSON request + //body can put an object here, and Mongo would read that object as a + //query expression instead of a value: the query would then match far + //more than the one view asked for, with no bound on how much of the + //drill collection it has to scan. + var scalarParams = ["view", "actionType", "segment"]; + for (var sp = 0; sp < scalarParams.length; sp++) { + if (!common.isQueryScalar(params.qstring[scalarParams[sp]])) { + common.returnMessage(params, 400, 'Bad request parameter: ' + scalarParams[sp]); + return false; + } + } + var actionType = params.qstring.actionType; if (!(device.minWidth >= 0) || !(device.maxWidth >= 0)) { diff --git a/test/unit-tests/api.utils.common.js b/test/unit-tests/api.utils.common.js index 7c28a1e9d71..22c9a636d94 100644 --- a/test/unit-tests/api.utils.common.js +++ b/test/unit-tests/api.utils.common.js @@ -359,6 +359,31 @@ describe("Common API utility functions", function() { }); }); + describe("isQueryScalar", function() { + it("accepts the values a scalar parameter legitimately arrives as", function() { + common.isQueryScalar("Home").should.equal(true); + common.isQueryScalar("").should.equal(true); + common.isQueryScalar("click").should.equal(true); + common.isQueryScalar(0).should.equal(true); + common.isQueryScalar(1777320900).should.equal(true); + common.isQueryScalar(true).should.equal(true); + }); + it("treats an absent parameter as a scalar, leaving that to the call site", function() { + common.isQueryScalar(undefined).should.equal(true); + common.isQueryScalar(null).should.equal(true); + }); + it("rejects an object, which Mongo would read as a query expression", function() { + common.isQueryScalar({ $ne: null }).should.equal(false); + common.isQueryScalar({ $regex: "." }).should.equal(false); + common.isQueryScalar({ $gt: "" }).should.equal(false); + common.isQueryScalar({}).should.equal(false); + }); + it("rejects an array", function() { + common.isQueryScalar([]).should.equal(false); + common.isQueryScalar(["Home", "About"]).should.equal(false); + }); + }); + describe("findUnsafeMongoOperator", function() { it("returns null for a clean query", function() { should.equal(common.findUnsafeMongoOperator({ lac: { $lt: 1777320900 } }), null);