diff --git a/CHANGELOG.md b/CHANGELOG.md index 37ace8a79ae..49e6f2bab48 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] `/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 - [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..b673931aedf 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'), @@ -935,7 +936,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(); + }); + }); +});