From adc9f3e90baacdfa77dcce1a63b25fee1c9aa59b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 18 Aug 2026 14:35:36 +0300 Subject: [PATCH] security(star-rating): forward only the widget's own parameters from /i/feedback/input Backport of #7952 to release.24.05. /i/feedback/input accepts a feedback submission from the web widget, which cannot compute a checksum because it does not hold the app's salt, so the handler replays the request into the generic /i processor with no_checksum set. It built that replayed request from the caller's entire query string: url: "/i?" + ob.params.href.split("/i/feedback/input?")[1] The handler's only check is on the events parameter, which has to be a single [CLY]_star_rating event. Every other parameter was forwarded untouched and then processed with checksum verification disabled, so a caller holding just the public app key could append unrelated write parameters and have them accepted unsigned on an app that has a checksum salt configured. old_device_id reaches appUsers.merge(), and token_session reaches the push token binding; begin_session, user_details, consent and crash ride along the same way. Rebuild the forwarded query from the parameters the widget actually sends (events, app_key, device_id, sdk_name, sdk_version, timestamp, hour, dow, app_version) and drop everything else, so the star rating submission keeps working while any other operation has to go through /i and satisfy the checksum. Values are URL encoded, so a parameter value cannot inject a second parameter, and non scalar values are dropped rather than stringified, since a JSON body can put an object in a query string parameter. The helper lives in api/input-utils.js so it can be unit tested; the existing plugin tests only vary device_id and never sent old_device_id here, so they are unaffected. Note this is the only unauthenticated no_checksum forwarder: the sole other no_checksum caller is api/utils/taskmanager.js, which replays a stored task URL created by an authenticated user. Reported through the security bug bounty programme (received 2026-08-18). Co-Authored-By: Claude Opus 4.8 --- CHANGELOG.md | 3 + plugins/star-rating/api/api.js | 8 +- plugins/star-rating/api/input-utils.js | 52 ++++++++ test/unit-tests/star-rating.input-utils.js | 139 +++++++++++++++++++++ 4 files changed, 200 insertions(+), 2 deletions(-) create mode 100644 plugins/star-rating/api/input-utils.js create mode 100644 test/unit-tests/star-rating.input-utils.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 7703fab7885..578efc93ec5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ Fixes: Enterprise Fixes: - [data-manager] Fixed editing an event whose key contains `&` creating undeletable duplicate rows in the events table +Security Fixes: +- [star-rating] `/i/feedback/input` now forwards only the parameters the feedback widget sends. Because that endpoint replays its request with checksum verification disabled, unrelated write parameters supplied by the caller (such as `old_device_id`, which merges app users, or `token_session`, which binds a push token) were previously processed without a checksum on apps that have a checksum salt configured + ## Version 24.05.51 Fixes: diff --git a/plugins/star-rating/api/api.js b/plugins/star-rating/api/api.js index 4ca7d12cc21..b7a37c17be5 100644 --- a/plugins/star-rating/api/api.js +++ b/plugins/star-rating/api/api.js @@ -7,7 +7,8 @@ var exported = {}, plugins = require('../../pluginManager.js'), { validateCreate, validateRead, validateUpdate, validateDelete, validateGlobalAdmin, validateAppAdmin } = require('../../../api/utils/rights.js'), countlyFs = require('../../../api/utils/countlyFs.js'), - imageUtils = require('./image-utils.js'); + imageUtils = require('./image-utils.js'), + inputUtils = require('./input-utils.js'); var fetch = require('../../../api/parts/data/fetch.js'); var ejs = require("ejs"), fs = require('fs'), @@ -927,7 +928,10 @@ function uploadFile(myfile, id, callback) { no_checksum: true, //providing data in request object 'req': { - url: "/i?" + ob.params.href.split("/i/feedback/input?")[1] + //only the widget's own parameters: this runs with no_checksum, + //so forwarding the caller's whole query string would let extra + //parameters reach /i unsigned. See input-utils.js. + url: "/i?" + inputUtils.buildForwardedQuery(ob.params.qstring) }, //adding custom processing for API responses 'APICallback': function(err, responseData, headers, returnCode) { diff --git a/plugins/star-rating/api/input-utils.js b/plugins/star-rating/api/input-utils.js new file mode 100644 index 00000000000..17f2cd304da --- /dev/null +++ b/plugins/star-rating/api/input-utils.js @@ -0,0 +1,52 @@ +/** +* Helpers for the /i/feedback/input endpoint. +* @module plugins/star-rating/api/input-utils +*/ + +/** @lends module:plugins/star-rating/api/input-utils */ +var inputUtils = {}; + +/** +* The only parameters the feedback widget sends, and therefore the only ones +* /i/feedback/input forwards to /i. See the widget request in +* frontend/public/templates/feedback-popup.html. +*/ +inputUtils.FORWARDED_INPUT_PARAMS = ["events", "app_key", "device_id", "sdk_name", "sdk_version", "timestamp", "hour", "dow", "app_version"]; + +/** +* Rebuild the query string that /i/feedback/input forwards to /i, keeping only the +* feedback widget's own parameters. +* +* The forwarded request runs with no_checksum, so whatever is forwarded reaches /i +* without checksum verification. Forwarding the caller's original query string let a +* caller append unrelated parameters, for example old_device_id (which merges app +* users) or token_session (which binds a push token), and have them processed unsigned +* even when the app has a checksum salt configured. Rebuilding the query from the +* allowlist keeps the star rating working while everything else has to go through /i +* and satisfy the checksum. +* +* Values that are not scalars are dropped rather than stringified, because a JSON +* request body can put an object or array in a query string parameter. +* +* @param {object} qstring - query string object of the incoming request +* @returns {string} encoded query string to forward to /i +*/ +inputUtils.buildForwardedQuery = function(qstring) { + var parts = []; + if (!qstring) { + return ""; + } + inputUtils.FORWARDED_INPUT_PARAMS.forEach(function(key) { + var value = qstring[key]; + if (typeof value === "undefined" || value === null) { + return; + } + if (typeof value !== "string" && typeof value !== "number" && typeof value !== "boolean") { + return; + } + parts.push(encodeURIComponent(key) + "=" + encodeURIComponent(value)); + }); + return parts.join("&"); +}; + +module.exports = inputUtils; diff --git a/test/unit-tests/star-rating.input-utils.js b/test/unit-tests/star-rating.input-utils.js new file mode 100644 index 00000000000..9219fd2d3f7 --- /dev/null +++ b/test/unit-tests/star-rating.input-utils.js @@ -0,0 +1,139 @@ +var should = require("should"); +var inputUtils = require("../../plugins/star-rating/api/input-utils.js"); + +// /i/feedback/input forwards its request to /i with no_checksum, so only the feedback +// widget's own parameters may be forwarded. Anything else would reach /i unsigned and +// bypass a configured checksum salt. +var STAR_RATING_EVENT = JSON.stringify([{ + key: "[CLY]_star_rating", + count: 1, + segmentation: { rating: 5, widget_id: "5f8b1c2d3e4f5a6b7c8d9e0f" } +}]); + +/** +* Parse a forwarded query string into a plain object. +* @param {string} query - forwarded query string +* @returns {object} decoded parameters +*/ +function parseQuery(query) { + var out = {}; + if (!query) { + return out; + } + query.split("&").forEach(function(pair) { + var eq = pair.indexOf("="); + var key = decodeURIComponent(pair.substring(0, eq)); + out[key] = decodeURIComponent(pair.substring(eq + 1)); + }); + return out; +} + +describe("star-rating input-utils", function() { + + describe("buildForwardedQuery", function() { + it("forwards every parameter the feedback widget sends", function(done) { + var widgetRequest = { + events: STAR_RATING_EVENT, + app_key: "APP_KEY", + device_id: "device-1", + sdk_name: "javascript_native_web", + sdk_version: "25.4.0", + timestamp: "1755500000000", + hour: "10", + dow: "2", + app_version: "5.5" + }; + var forwarded = parseQuery(inputUtils.buildForwardedQuery(widgetRequest)); + Object.keys(widgetRequest).forEach(function(key) { + should(forwarded[key]).equal(widgetRequest[key]); + }); + done(); + }); + + it("round-trips the events payload unchanged", function(done) { + var forwarded = parseQuery(inputUtils.buildForwardedQuery({ + events: STAR_RATING_EVENT, + app_key: "APP_KEY", + device_id: "device-1" + })); + should(forwarded.events).equal(STAR_RATING_EVENT); + done(); + }); + + it("drops old_device_id so the endpoint cannot merge app users unsigned", function(done) { + var forwarded = inputUtils.buildForwardedQuery({ + events: STAR_RATING_EVENT, + app_key: "APP_KEY", + device_id: "attacker-device", + old_device_id: "victim-device" + }); + should(forwarded.indexOf("old_device_id")).equal(-1); + should(forwarded.indexOf("victim-device")).equal(-1); + done(); + }); + + it("drops push token parameters so the endpoint cannot rebind a token unsigned", function(done) { + var forwarded = inputUtils.buildForwardedQuery({ + events: STAR_RATING_EVENT, + app_key: "APP_KEY", + device_id: "attacker-device", + token_session: "1", + token: "attacker-push-token" + }); + should(forwarded.indexOf("token_session")).equal(-1); + should(forwarded.indexOf("attacker-push-token")).equal(-1); + done(); + }); + + it("drops other write parameters that would otherwise ride along", function(done) { + var forwarded = inputUtils.buildForwardedQuery({ + events: STAR_RATING_EVENT, + app_key: "APP_KEY", + device_id: "device-1", + begin_session: "1", + end_session: "1", + user_details: JSON.stringify({ name: "someone" }), + consent: JSON.stringify({ push: true }), + crash: JSON.stringify({ _error: "x" }), + metrics: JSON.stringify({ _os: "iOS" }), + ip_address: "203.0.113.1" + }); + ["begin_session", "end_session", "user_details", "consent", "crash", "metrics", "ip_address"].forEach(function(key) { + should(forwarded.indexOf(key)).equal(-1); + }); + done(); + }); + + it("drops non scalar values instead of stringifying them", function(done) { + var forwarded = inputUtils.buildForwardedQuery({ + events: STAR_RATING_EVENT, + app_key: "APP_KEY", + device_id: { $ne: null } + }); + should(forwarded.indexOf("device_id")).equal(-1); + should(forwarded.indexOf("object")).equal(-1); + done(); + }); + + it("encodes values so a parameter cannot inject another one", function(done) { + var forwarded = inputUtils.buildForwardedQuery({ + events: STAR_RATING_EVENT, + app_key: "APP_KEY", + device_id: "d1&old_device_id=victim-device" + }); + should(forwarded.indexOf("&old_device_id=")).equal(-1); + var forwardedParams = parseQuery(forwarded); + should(forwardedParams.device_id).equal("d1&old_device_id=victim-device"); + should(forwardedParams).not.have.property("old_device_id"); + done(); + }); + + it("skips absent parameters and tolerates an empty query", function(done) { + var forwarded = inputUtils.buildForwardedQuery({ events: STAR_RATING_EVENT, app_key: "APP_KEY" }); + should(forwarded.indexOf("device_id")).equal(-1); + should(inputUtils.buildForwardedQuery({})).equal(""); + should(inputUtils.buildForwardedQuery(null)).equal(""); + done(); + }); + }); +});