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

[ar2rsawseen]

What

params.qstring values are not guaranteed to be strings. api/api.js fills params.qstring straight from formidable's fields, and with the formidable 2.1.3 this repo ships, a POST body sent as application/json puts nested objects there. A body like {"view": {"$ne": null}} leaves params.qstring.view an object rather than the string the endpoint expects. Form-urlencoded bracket syntax (view[$ne]=x) 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 data it has to scan.

The heatmap case is the expensive one. getHeatmap builds one $match from view, actionType and segment, and when use_union_with is on it $unionWiths 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. That is the main motivation for this change.

How

Adds common.isQueryScalar(value): true for strings, numbers, booleans, null and undefined, false for objects and arrays. null/undefined stay scalars on purpose, 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 considered first and are not the right tool here. They validate parameters that are queries in their own right, and they only reject the JS-executing operators, so they accept $ne and $regex by design. Endpoints that legitimately take a whole user query already go through them and are untouched.

Compatibility

Strings and numbers pass through unchanged, so legitimate callers see no difference. /o/actions still takes view as a string URL, actionType as "click" or "scroll" and segment as a string. Only non-scalars are refused, with a 400 naming the parameter, the same way device and period were already checked in the same function.

Sites surveyed

The whole class was swept across api/ and every plugin api/ directory, resolving one level of local-variable indirection rather than relying on same-line matches. Verdict per site:

Fixed

Site	Parameter(s)
plugins/views/api/api.js getHeatmap	view, actionType, segment
plugins/star-rating/api/api.js /o/feedback/data	widget_id, version, platform, uid
plugins/star-rating/api/api.js /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

Already validated, left alone

• api/utils/rights.js (7 sites): each does params.qstring.api_key = params.qstring.api_key + "" immediately before building the query.
• Anything keyed on app_id or user_id: processRequest rejects a non-24-length value up front, which covers objects.
• app_key, device_id, old_device_id: processRequest casts these to strings for every request.
• plugins/crashes/api/api.js method=crashes (group): cast with + "" on the line above.
• api/parts/data/fetch.js jobDetails (name): cast with + "" on the line above.
• plugins/populator/api/api.js (name, template_id): name goes through common.validateArgs with type: "String" first; template_id goes through common.db.ObjectID in a try/catch first.
• plugins/alerts/api/api.js, plugins/star-rating/api/api.js widget delete, api/parts/mgmt/users.js note edit/delete, api/parts/mgmt/apps.js app plugins, plugins/compare/api/api.js: the value reaches common.db.ObjectID (or a length check) before any query.
• api/parts/mgmt/cms.js (lines 202 and 289), /o/tasks and the app_users query path: these take a user-supplied query by design and already validate it with common.parseUserQuery. plugins/dbviewer/api/api.js validates its filter and sort with common.findUnsafeMongoOperator (line 235).
• plugins/dbviewer/api/api.js line 133 (document) is a genuine instance of the pattern and was still left alone: the same endpoint accepts an arbitrary filter and sort by design, and a non-global-admin document lookup is additionally constrained by getBaseAppFilter, so constraining document would grant nothing the endpoint does not already grant. Worth revisiting if the dbviewer query surface is ever narrowed.

False positives

• Values that only ever reach string concatenation: segment/segmentVal in the /o/views branch (hashed into a collection name and into $d.… field paths), metric in fetch.metricToCollection, the populator $regex prefix built with "^" + app_id + "_" + template_id.
• $set payloads rather than query filters: homeSettings, sdk.name/sdk.version/did/t/tz in common.js, consent.* in compliance-hub, hc in sdk, event_map/omitted_segments in requestProcessor. Those write request data into a document, which is what they are for, and are not operator positions.
• sEcho, iTotalRecords and friends: response fields, not query fields.
• $in, $regex and $text: {$search: …} positions (event_groups ids, views sSearch, star-rating sSearch, fetch.js events): Mongo rejects a non-array $in and a non-string $regex/$search, so these error out rather than widening. Left as they are to keep this change to the one pattern.
• sort/projection keys are all looked up in fixed column arrays, so no request value becomes a key.

Tests

• test/unit-tests/api.utils.common.js: unit tests for common.isQueryScalar. These run in test-api-core and pass there.

• plugins/views/tests/heatmaps.js: a new case seeds a second action on a different view, then asserts that a string view still returns only that view's action and that an object view is refused with 400 instead of returning both. A second case covers non-scalar actionType and segment, and confirms a string segment is still accepted.

  Worth knowing before reviewing: this file does not currently run in CI. plugins/views/tests/index.js has //require('./heatmaps.js');, disabled in 01703b8 (2025-04-29). As written the suite cannot pass in this repo's CI anyway: it reads resp.body.plugins.drill from /o/apps/plugins and connects to countly_drill, neither of which exists here. The new cases were left in that file because it is where the other /o/actions cases live, so they run wherever that suite is run against a drill-enabled instance. Re-enabling the suite is a separate decision and is not part of this PR.

Verification

• node --check on all 9 changed files: clean.
• eslint -c .eslintrc.json on all 9 changed files: 0 errors (2 pre-existing Promise.each warnings in plugins/views/api/api.js, on lines this PR does not touch).
• mocha test/unit-tests/api.utils.common.js: 41 passing, including the 4 new ones.
• The premise was confirmed against the repo's own formidable 2.1.3: a JSON body with a nested object yields typeof fields.view === "object".
• The heatmap cases in plugins/views/tests/heatmaps.js need a drill-enabled instance running this branch and were not executed.
• test-api-plugins failed once on plugins/web/tests.js "should have Chrome in browser metrics", a UA and client-hints parsing assertion, and passed on re-run with no change to the branch. Nothing in this PR touches UA parsing, the browser metric, or any write path, and the same job passed first time on the 24.05 backport of this change ([fix][core] validate scalar request parameter types before using them in queries (24.05) #7938) and on other current PRs. All checks are green.