From 4e97abff53c01c0224d9c71c9f88a401ecb03839 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 17:54:45 +0300 Subject: [PATCH] [fix][core] validate scalar request parameter types before using them in queries params.qstring values are not guaranteed to be strings. api/api.js fills params.qstring straight from formidable's fields, and with formidable 2.1.3 a POST body sent as application/json puts nested objects there. So a body like {"view": {"$ne": null}} leaves params.qstring.view an object rather than the string the endpoint expects. Form-urlencoded bracket syntax does not do this, it keeps a literal string key, so JSON bodies are the case that matters. Several endpoints then use such a parameter as a plain value inside a Mongo match document. In a value position Mongo reads an object as a query expression, so an equality match on one document becomes a match on many. Two consequences: the query returns rows the endpoint never meant to return, and it loses the bound on how much it has to scan. The heatmap case is the expensive one: getHeatmap builds one $match from view, actionType and segment, then, when use_union_with is on, $unionWith's the older drill collection into the same pipeline over a caller-chosen period. Widening that match turns a single cheap request into a very large aggregation on the drill database. Adds common.isQueryScalar and applies it where a scalar parameter reaches a query with no type check: plugins/views/api/api.js getHeatmap: view, actionType, segment plugins/star-rating/api/api.js /o/feedback/data: widget_id, version, platform, uid /o/feedback/widgets: is_active plugins/crashes/api/api.js method=user_crashes: uid api/parts/mgmt/users.js fetchNotes: note_type api/utils/requestProcessor.js /i/token/delete: tokenid plugins/systemlogs/api/api.js member lookup: api_key Strings and numbers pass through unchanged, so legitimate callers are unaffected. /o/actions in particular still takes view as a string URL, actionType as "click" or "scroll" and segment as a string; only non-scalars are refused, with 400 and the parameter name, the same way device and period were already checked there. null and undefined stay scalars so the existing truthiness checks at each call site keep deciding whether an absent parameter belongs in the query at all. common.parseUserQuery and common.findUnsafeMongoOperator were not the right tool here: they validate parameters that are queries in their own right and only reject the JS-executing operators, so they accept $ne and $regex by design. Endpoints that legitimately take a whole user query (cms, dbviewer, /o/tasks) already go through them and are left alone. Co-Authored-By: Claude Opus 5 --- api/parts/mgmt/users.js | 6 +++ api/utils/common.js | 29 +++++++++++++ api/utils/requestProcessor.js | 9 ++++ plugins/crashes/api/api.js | 8 ++++ plugins/star-rating/api/api.js | 18 ++++++++ plugins/systemlogs/api/api.js | 5 ++- plugins/views/api/api.js | 14 ++++++ plugins/views/tests/heatmaps.js | 66 +++++++++++++++++++++++++++++ test/unit-tests/api.utils.common.js | 25 +++++++++++ 9 files changed, 179 insertions(+), 1 deletion(-) 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);