From 1b5db2bfba69fda145743053ef5549cc38f00ac5 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 17:59:41 +0300 Subject: [PATCH] [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);