From 1b5db2bfba69fda145743053ef5549cc38f00ac5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 17:59:41 +0300 Subject: [PATCH 1/2] [fix][core] validate scalar request parameter types before using them in queries (24.05) params.qstring values are not guaranteed to be strings. api/api.js fills params.qstring straight from formidable's fields, and 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 query 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. getHeatmap is the expensive one: it builds one query from view, actionType and segment and runs it against the drill action collection over a caller-chosen period, so widening the match turns a single cheap request into a full scan of that collection. 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. Backport of #7937. 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 ++++++++++++++ test/unit-tests/api.utils.common.js | 25 +++++++++++++++++++++++++ 8 files changed, 113 insertions(+), 1 deletion(-) 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); From 1d7edf6863cf4a431e85bf14384358b967e738ac Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 14:33:13 +0300 Subject: [PATCH 2/2] [fix][security][core] guard device_id, and require a concrete heatmap view and action type Two gaps in the same fix. /o/feedback/data assigns params.qstring.device_id straight into the query and it was not in the guarded list, so a JSON body could still put an operator there. Added, along with sSearch, which lands in $text.$search and would otherwise take an object. /o/actions is the more interesting one. isQueryScalar accepts null and undefined on purpose - whether an absent parameter belongs in the query is the call site's decision - and here it was not being made: actionType is assigned into matchQuery['sg.type'] unconditionally, and an equality predicate against null also matches every document where the field is absent. Ingestion accepts [CLY]_action events with no sg.type, so omitting actionType selected those rows over the caller's whole period rather than selecting nothing, and the $unionWith across the drill collections had nothing bounding what it scanned. Being a scalar was never the property that mattered for this parameter. actionType now has to be "click" or "scroll", the only two values heatmap.js ever sends. The same is true of view one field over, so it is fixed with it: the click branch assigns matchQuery['up.lv'] = params.qstring.view and the scroll branch assigns matchQuery['sg.view'], both unconditionally, so an omitted view matched users with no recorded last view instead of matching nothing. It now has to be a non empty string. Neither branch has a meaningful "all views" reading to preserve. Tests: omitted, null and unsupported actionType, omitted and empty view, and both supported types still accepted. They live in plugins/views/tests/heatmaps.js, which only exists on master - the platform copy is heatmaps.js.old and 24.05 has no views test directory - so the other two carry the code fix and the isQueryScalar unit cases. --- plugins/star-rating/api/api.js | 2 +- plugins/views/api/api.js | 20 +++++++++++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index cf829cecb3b..919a6d95421 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -1259,7 +1259,7 @@ function uploadFile(myfile, id, callback) { //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']; + var scalarParams = ['widget_id', 'version', 'platform', 'device_id', 'sSearch', 'uid']; for (var s = 0; s < scalarParams.length; s++) { if (!common.isQueryScalar(params.qstring[scalarParams[s]])) { common.returnMessage(params, 400, 'Invalid parameter: ' + scalarParams[s]); diff --git a/plugins/views/api/api.js b/plugins/views/api/api.js index 3b70516a3cf..70e150a81b0 100644 --- a/plugins/views/api/api.js +++ b/plugins/views/api/api.js @@ -1366,7 +1366,7 @@ const escapedViewSegments = { "name": true, "segment": true, "height": true, "wi //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"]; + var scalarParams = ["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]); @@ -1374,6 +1374,24 @@ const escapedViewSegments = { "name": true, "segment": true, "height": true, "wi } } + //view and actionType get a stronger check than isQueryScalar, which accepts null + //and undefined on purpose: whether an absent parameter belongs in the query is + //the caller's decision, and here it does not. Both are assigned into the match + //unconditionally below, and an equality predicate against null also matches every + //document where the field is absent. Ingestion accepts [CLY]_action events with + //no sg.type and users with no up.lv, so an omitted value selects those rather + //than nothing, and the $unionWith across the drill collections then has no bound + //on how much it scans. Both callers in heatmap.js always send a concrete value. + var ACTION_TYPES = ["click", "scroll"]; + if (ACTION_TYPES.indexOf(params.qstring.actionType) === -1) { + common.returnMessage(params, 400, 'Bad request parameter: actionType'); + return false; + } + if (typeof params.qstring.view !== "string" || !params.qstring.view.length) { + common.returnMessage(params, 400, 'Bad request parameter: view'); + return false; + } + var actionType = params.qstring.actionType; if (!(device.minWidth >= 0) || !(device.maxWidth >= 0)) {