[fix][core] validate scalar request parameter types before using them in queries

api/parts/mgmt/users.js

@@ -1122,6 +1122,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;
     }
 

api/utils/common.js

@@ -2570,6 +2570,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

api/utils/requestProcessor.js

@@ -2519,6 +2519,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 + ""

plugins/crashes/api/api.js

@@ -1165,6 +1165,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});

plugins/star-rating/api/api.js

@@ -1264,6 +1264,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;
         }
@@ -1423,6 +1435,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;
             }
 

plugins/systemlogs/api/api.js

@@ -467,7 +467,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) {

plugins/views/api/api.js

@@ -1360,6 +1360,20 @@ const escapedViewSegments = { "name": true, "segment": true, "height": true, "wi
             log.e('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 pipeline would then match
+        //far more than the one view asked for, and the $unionWith across the
+        //drill collections would have no bound on how much it scans.
+        const scalarParams = ['view', 'actionType', 'segment'];
+        for (let i = 0; i < scalarParams.length; i++) {
+            if (!common.isQueryScalar(params.qstring[scalarParams[i]])) {
+                common.returnMessage(params, 400, 'Bad request parameter: ' + scalarParams[i]);
+                return false;
+            }
+        }
+
         const actionType = params.qstring.actionType;
 
         if (!(device.minWidth >= 0) || !(device.maxWidth >= 0)) {

plugins/views/tests/heatmaps.js

@@ -88,6 +88,72 @@ describe('Heatmap', async() => {
         should(data[0].sg).eql(clickData);
     });
 
+    it('does not widen the match when view is not a scalar', async() => {
+        const db = await pluginManager.dbConnection('countly_drill');
+        const baseQuery = {
+            api_key: API_KEY_ADMIN,
+            app_id: APP_ID,
+            app_key: APP_KEY,
+            period: JSON.stringify([moment('2010-01-01').valueOf(), moment('2010-01-31').valueOf()]),
+            device: JSON.stringify({ type: 'all', displayText: 'All', minWidth: 0, maxWidth: 10240 }),
+            actionType: 'click',
+        };
+
+        // a second action on a different view, so a widened match would return
+        // two rows where a match on one view returns one
+        await db.collection('drill_events').insertOne({
+            did: 'heatmap_test',
+            a: APP_ID,
+            e: '[CLY]_action',
+            sg: { ...clickData, domain: 'https://doma.in', view: 'About' },
+            ts: moment('2010-01-02').valueOf(),
+            up: { lv: 'About' },
+        });
+
+        // a string view still returns only that view's action
+        const scoped = await request.post('/o/actions').send({ ...baseQuery, view: 'Home' });
+        should(scoped.status).equal(200);
+        should(scoped.body.data.length).equal(1);
+        should(scoped.body.data[0].sg).eql(clickData);
+
+        // an object view is refused rather than run as a query expression
+        const widened = await request.post('/o/actions').send({ ...baseQuery, view: { $ne: null } });
+        should(widened.status).equal(400);
+        should(widened.body.result).equal('Bad request parameter: view');
+        should.not.exist(widened.body.data);
+
+        await db.collection('drill_events').remove({ did: 'heatmap_test', 'up.lv': 'About' });
+
+        db.close();
+    });
+
+    it('refuses a non-scalar actionType or segment', async() => {
+        const baseQuery = {
+            api_key: API_KEY_ADMIN,
+            app_id: APP_ID,
+            app_key: APP_KEY,
+            view: 'Home',
+            period: JSON.stringify([moment('2010-01-01').valueOf(), moment('2010-01-31').valueOf()]),
+            device: JSON.stringify({ type: 'all', displayText: 'All', minWidth: 0, maxWidth: 10240 }),
+        };
+
+        const badActionType = await request.post('/o/actions')
+            .send({ ...baseQuery, actionType: { $ne: 'scroll' } });
+        should(badActionType.status).equal(400);
+        should(badActionType.body.result).equal('Bad request parameter: actionType');
+
+        const badSegment = await request.post('/o/actions')
+            .send({ ...baseQuery, actionType: 'click', segment: { $ne: null } });
+        should(badSegment.status).equal(400);
+        should(badSegment.body.result).equal('Bad request parameter: segment');
+
+        // a string segment is still accepted, it just matches nothing here
+        const goodSegment = await request.post('/o/actions')
+            .send({ ...baseQuery, actionType: 'click', segment: 'nosuchsegment' });
+        should(goodSegment.status).equal(200);
+        should(goodSegment.body.data.length).equal(0);
+    });
+
     it('gets heatmap data from old drill_events collection if union_with is true', async() => {
         const db = await pluginManager.dbConnection('countly_drill');
         const oldCollectionName = 'drill_events' + crypto.createHash('sha1').update('[CLY]_action' + APP_ID).digest('hex');

test/unit-tests/api.utils.common.js

@@ -425,6 +425,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);