diff --git a/api/parts/mgmt/users.js b/api/parts/mgmt/users.js index 5510c534aa6..1edce678ea2 100644 --- a/api/parts/mgmt/users.js +++ b/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; } diff --git a/api/utils/common.js b/api/utils/common.js index 05119c59521..02ede7f1c30 100644 --- a/api/utils/common.js +++ b/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 diff --git a/api/utils/requestProcessor.js b/api/utils/requestProcessor.js index 3287fe1c7dc..241f8c83adf 100644 --- a/api/utils/requestProcessor.js +++ b/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 + "" diff --git a/plugins/crashes/api/api.js b/plugins/crashes/api/api.js index 98147f4ea58..5667e553203 100644 --- a/plugins/crashes/api/api.js +++ b/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}); diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index c3626c52fd4..b780618888c 100644 --- a/plugins/star-rating/api/api.js +++ b/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; } diff --git a/plugins/systemlogs/api/api.js b/plugins/systemlogs/api/api.js index 332d5e2f7d5..ff040024ec6 100644 --- a/plugins/systemlogs/api/api.js +++ b/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) { diff --git a/plugins/views/api/api.js b/plugins/views/api/api.js index 41ca1c35c6b..1da1e38e6c0 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 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)) { diff --git a/plugins/views/tests/heatmaps.js b/plugins/views/tests/heatmaps.js index 77e92f6e0d1..e10cc10bbfd 100644 --- a/plugins/views/tests/heatmaps.js +++ b/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'); diff --git a/test/unit-tests/api.utils.common.js b/test/unit-tests/api.utils.common.js index b68bcc2a74b..88335913dfe 100644 --- a/test/unit-tests/api.utils.common.js +++ b/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);