security(star-rating): forward only the widget's own parameters from /i/feedback/input (24.05)

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:

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) {

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;

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();
+        });
+    });
+});