From 3d294aa3dba3609b67bbdb507b2fce7c85811778 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 17:18:27 +0300 Subject: [PATCH] security(star-rating): authorize the /o?method=star ratings read The star-rating dashboard read performed no authorization. Authentication on /o is per method: every core method calls validateUserForDataReadAPI itself, and the default branch hands the validators to plugins as helpers without calling them, so a plugin that claims a request is responsible for authorizing it. This branch claimed the request, returned true, and never called a validator, so the endpoint answered callers with no account, token or session, for any app_id they supplied. What it disclosed is the set of platform and application-version combinations that have received ratings for that application. Rating comments and the detailed feedback in /o/feedback/data were not affected; those reads are authorized. Wrap the branch in validateRead(params, FEATURE_NAME, ...), which is the same check the sibling reads in this file already apply (/o/feedback/data and /o/feedback/widgets). Authorization runs before the period parameter is validated, so an unauthorized caller cannot probe the endpoint through its error responses. app_id needs no extra guard here: the core /o case rejects a request without one, and this branch only concatenates it into a collection name, so there is no ObjectID conversion that could throw for a global admin whose validateRead call does not require app_id. The only caller is the dashboard Ratings page (plugins/star-rating/frontend/public/javascripts/countly.models.js, starRatingPlugin.requestPlatformVersion), which sends the session credential and an app_id the member has access to, so it is unaffected. Reported through the security bug bounty programme (received 2026-08-18). Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 1 + plugins/star-rating/api/api.js | 132 +++++++++++++++++---------------- 2 files changed, 70 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 37ace8a79ae..c586c206082 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ Enterprise Fixes: - [data-manager] Fixed editing an event whose key contains `&` creating undeletable duplicate rows in the events table Security Fixes: +- [star-rating] The `/o?method=star` ratings read now requires star-rating read access to the application it is asked about. It previously performed no authorization, so the platform and application-version combinations that had received ratings could be read for any application by a caller with no account, token or session - [hooks] Internal event hooks are now scoped to the apps the hook belongs to: app creation is a global-admin-only event, and remote-config, cohort, alert and hook-chaining events are only delivered when the event's app is one the hook is scoped to - [compliance-hub] The consents table now returns a fixed set of fields; a projection supplied on the request is no longer used to widen the response beyond the consent columns - [dashboards] Widgets are no longer copied when the copying user has no access to the apps they reference, and widget app ids are validated on widget create and update diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index c3626c52fd4..000fe5848c8 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -1577,79 +1577,85 @@ function uploadFile(myfile, id, callback) { plugins.register('/o', function(ob) { var params = ob.params; if (params.qstring.method === 'star') { - if (params.qstring.period) { - //check if period comes from datapicker - if (params.qstring.period.indexOf(",") !== -1) { - try { - params.qstring.period = JSON.parse(params.qstring.period); - } - catch (SyntaxError) { - common.returnMessage(params, 400, 'Bad request parameter: period'); - return true; - } - } - else { - switch (params.qstring.period) { - case "prevMonth": - case "month": - case "day": - case "yesterday": - case "hour": - break; - default: - if (!/([0-9]+)days/.test(params.qstring.period)) { + //this read is app scoped: require the caller to hold star-rating + //read access on app_id, the same check the sibling reads in this + //file apply. Authorize before validating parameters so that an + //unauthorized caller cannot probe the endpoint. + validateRead(params, FEATURE_NAME, function() { + if (params.qstring.period) { + //check if period comes from datapicker + if (params.qstring.period.indexOf(",") !== -1) { + try { + params.qstring.period = JSON.parse(params.qstring.period); + } + catch (SyntaxError) { common.returnMessage(params, 400, 'Bad request parameter: period'); return true; } - break; + } + else { + switch (params.qstring.period) { + case "prevMonth": + case "month": + case "day": + case "yesterday": + case "hour": + break; + default: + if (!/([0-9]+)days/.test(params.qstring.period)) { + common.returnMessage(params, 400, 'Bad request parameter: period'); + return true; + } + break; + } } } - } - else { - common.returnMessage(params, 400, 'Missing request parameter: period'); - return true; - } - countlyCommon.setPeriod(params.qstring.period, true); - var periodObj = countlyCommon.periodObj; - var collectionName = crypto.createHash('sha1').update('[CLY]_star_rating' + params.qstring.app_id).digest('hex'); - var id_prefix = params.qstring.app_id + "_" + collectionName + "_"; - var documents = []; - for (var i = 0; i < periodObj.reqZeroDbDateIds.length; i++) { - documents.push(id_prefix + "no-segment_" + periodObj.reqZeroDbDateIds[i]); - for (var m = 0; m < common.base64.length; m++) { - documents.push(id_prefix + "no-segment_" + periodObj.reqZeroDbDateIds[i] + "_" + common.base64[m]); + else { + common.returnMessage(params, 400, 'Missing request parameter: period'); + return true; } - } - common.db.collection("events_data").find({ - '_id': { - $in: documents + countlyCommon.setPeriod(params.qstring.period, true); + var periodObj = countlyCommon.periodObj; + var collectionName = crypto.createHash('sha1').update('[CLY]_star_rating' + params.qstring.app_id).digest('hex'); + var id_prefix = params.qstring.app_id + "_" + collectionName + "_"; + var documents = []; + for (var i = 0; i < periodObj.reqZeroDbDateIds.length; i++) { + documents.push(id_prefix + "no-segment_" + periodObj.reqZeroDbDateIds[i]); + for (var m = 0; m < common.base64.length; m++) { + documents.push(id_prefix + "no-segment_" + periodObj.reqZeroDbDateIds[i] + "_" + common.base64[m]); + } } - }).toArray(function(err, docs) { - if (!err) { - var result = {}; - docs.forEach(function(doc) { - if (!doc.meta) { - doc.meta = {}; - } - if (!doc.meta.platform_version_rate) { - doc.meta.platform_version_rate = []; - } - if (doc.meta_v2 && doc.meta_v2.platform_version_rate) { - common.arrayAddUniq(doc.meta.platform_version_rate, Object.keys(doc.meta_v2.platform_version_rate)); - } - doc.meta.platform_version_rate.forEach(function(item) { - var data = item.split('**'); - if (result[data[0]] === undefined) { - result[data[0]] = []; + common.db.collection("events_data").find({ + '_id': { + $in: documents + } + }).toArray(function(err, docs) { + if (!err) { + var result = {}; + docs.forEach(function(doc) { + if (!doc.meta) { + doc.meta = {}; + } + if (!doc.meta.platform_version_rate) { + doc.meta.platform_version_rate = []; } - if (result[data[0]].indexOf(data[1]) === -1) { - result[data[0]].push(data[1]); + if (doc.meta_v2 && doc.meta_v2.platform_version_rate) { + common.arrayAddUniq(doc.meta.platform_version_rate, Object.keys(doc.meta_v2.platform_version_rate)); } + doc.meta.platform_version_rate.forEach(function(item) { + var data = item.split('**'); + if (result[data[0]] === undefined) { + result[data[0]] = []; + } + if (result[data[0]].indexOf(data[1]) === -1) { + result[data[0]].push(data[1]); + } + }); }); - }); - common.returnOutput(params, result); - return true; - } + common.returnOutput(params, result); + return true; + } + }); }); return true; }