Merge branch 'master' into QT-426

Author: savascountly

SECURITY.md

@@ -36,3 +36,5 @@ The following are out of scope. They may still be reported, and configuration is
 9. **Hooks custom-code effects.** The Hooks plugin's custom-code effect runs operator-supplied JavaScript and is being migrated to a stronger isolation model (`isolated-vm`) in an upcoming release, which removes the existing execution surface entirely. Issues that depend on the behaviour of the current custom-code sandbox (for example escaping or abusing the bundled sandbox's built-in helpers) are out of scope. Note that the Hooks plugin already requires an authenticated account with the relevant per-app hooks permission, and the custom-code effect executes code the operator themselves configured.
 
 10. **Instances of a vulnerability class already under active remediation.** Findings that are additional instances of a vulnerability class we are already remediating — including work visible in an open or in-progress pull request, a public branch, or another not-yet-released fix — are considered part of that known, ongoing effort and are not separately eligible. Enumerating sibling occurrences of an issue from our published or in-progress remediation is not an independent discovery. Independently discovered issues remain welcome.
+
+11. **Cross-site scripting (XSS) without a working proof of concept.** XSS reports that do not demonstrate actual script execution in an authenticated dashboard session are out of scope. Pointing at a potential sink (for example a `v-html` binding or a DOM write) is not sufficient on its own, since the value reaching a sink may already be neutralized elsewhere in the request handling or rendering pipeline. XSS with a working end-to-end proof of concept — including DOM-based XSS that originates from the URL or other client-controlled input — is in scope and welcome.

api/parts/data/exports.js

@@ -500,6 +500,51 @@ function isObjectId(id) {
 * @param {string} [options.filename] - name of the file to output, by default auto generated
 * @param {function} options.output - callback function where to pass data, by default outputs as file based on type
 */
+
+/**
+* Credential fields stripped from privileged collections on export, mirroring
+* the DB Viewer read paths so the same data cannot be obtained through export.
+*/
+var EXPORT_REDACTIONS = {
+    "members": ["password", "api_key", "two_factor_auth"],
+    "auth_tokens": ["_id"]
+};
+
+/**
+* Whether documents from the given collection require credential redaction on export
+* @param {string} collection - collection name
+* @returns {boolean} true if the collection is redacted on export
+*/
+exports.redactsExportCollection = function(collection) {
+    return Object.prototype.hasOwnProperty.call(EXPORT_REDACTIONS, collection);
+};
+
+/**
+* Remove/mask credential fields from a document before it is written to an export
+* @param {string} collection - collection the document belongs to
+* @param {object} doc - document to redact (mutated and returned)
+* @returns {object} the redacted document
+*/
+exports.redactExportDoc = function(collection, doc) {
+    if (!doc || !exports.redactsExportCollection(collection)) {
+        return doc;
+    }
+    var fields = EXPORT_REDACTIONS[collection];
+    for (var i = 0; i < fields.length; i++) {
+        if (fields[i] === "_id") {
+            // _id is normally present (fromDatabase keeps it unless explicitly
+            // excluded via projection), so mask its value rather than deleting it
+            if (typeof doc._id !== "undefined") {
+                doc._id = "***redacted***";
+            }
+        }
+        else {
+            delete doc[fields[i]];
+        }
+    }
+    return doc;
+};
+
 exports.fromDatabase = function(options) {
     options.db = options.db || common.db;
     options.query = options.query || {};
@@ -566,6 +611,15 @@ exports.fromDatabase = function(options) {
                 options.query._id = common.db.ObjectID(options.query._id);
             }
             var cursor = options.db.collection(options.collection).find(options.query, {"projection": options.projection});
+            // Strip credential material from privileged collections before it
+            // can reach an export file, mirroring the DB Viewer read paths. This
+            // runs at the cursor level so it applies regardless of output type
+            // (json/csv/xls) and regardless of the caller supplied projection.
+            if (exports.redactsExportCollection(options.collection)) {
+                cursor = cursor.map(function(doc) {
+                    return exports.redactExportDoc(options.collection, doc);
+                });
+            }
             if (options.sort) {
                 cursor.sort(options.sort);
             }

api/parts/mgmt/users.js

@@ -286,9 +286,15 @@ usersApi.createUser = async function(params) {
                         mail.sendToNewMemberLink(member[0], prid);
                     });
 
+                    // Broadcast a credential-free copy of the new member to
+                    // event listeners (e.g. hooks): the member's password hash
+                    // and api_key must never be forwarded into a hook payload.
+                    var createdMemberEventData = Object.assign({}, member[0]);
+                    delete createdMemberEventData.password;
+                    delete createdMemberEventData.api_key;
                     plugins.dispatch("/i/users/create", {
                         params: params,
-                        data: member[0]
+                        data: createdMemberEventData
                     });
                     delete member[0].password;
 
@@ -558,10 +564,17 @@ usersApi.updateUser = async function(params) {
             common.db.collection('members').findOne({ '_id': common.db.ObjectID(params.qstring.args.user_id) }, function(err2, member) {
                 if (member && !err2) {
                     updatedMember._id = params.qstring.args.user_id;
+                    // never forward credentials into event payloads (e.g. hooks)
+                    var updatedMemberEventData = Object.assign({}, updatedMember);
+                    delete updatedMemberEventData.password;
+                    delete updatedMemberEventData.api_key;
+                    var memberBeforeEventData = Object.assign({}, memberBefore);
+                    delete memberBeforeEventData.password;
+                    delete memberBeforeEventData.api_key;
                     plugins.dispatch("/i/users/update", {
                         params: params,
-                        data: updatedMember,
-                        member: memberBefore
+                        data: updatedMemberEventData,
+                        member: memberBeforeEventData
                     });
                     if (params.qstring.args.send_notification && passwordNoHash) {
                         mail.sendToUpdatedMember(member, passwordNoHash);
@@ -616,10 +629,14 @@ usersApi.deleteUser = async function(params) {
         else {
             const user = await common.db.collection('members').findOne({ '_id': common.db.ObjectID(userIds[i]) });
             const promisifiedDispatch = function(prms, data) {
+                // never forward credentials into event payloads (e.g. hooks)
+                var safeData = Object.assign({}, data);
+                delete safeData.password;
+                delete safeData.api_key;
                 return new Promise((resolve, reject) => {
                     plugins.dispatch("/i/users/delete", {
                         params: prms,
-                        data,
+                        data: safeData,
                     }, async(__, otherPluginResults) => {
                         const rejectReasons = otherPluginResults.reduce((acc, result) => {
                             if (result.status === "rejected") {
@@ -813,6 +830,10 @@ usersApi.deleteOwnAccount = function(params) {
             };
 
             if (member) {
+                // never forward credentials into event payloads (e.g. hooks)
+                var memberEventData = Object.assign({}, member);
+                delete memberEventData.password;
+                delete memberEventData.api_key;
                 if (member.global_admin) {
                     common.db.collection('members').count({'global_admin': true}, function(err2, count) {
                         if (err2) {
@@ -825,15 +846,15 @@ usersApi.deleteOwnAccount = function(params) {
                         else {
                             plugins.dispatch("/i/users/delete", {
                                 params: params,
-                                data: member
+                                data: memberEventData
                             }, dispatchDeleteCallback);
                         }
                     });
                 }
                 else {
                     plugins.dispatch("/i/users/delete", {
                         params: params,
-                        data: member
+                        data: memberEventData
                     }, dispatchDeleteCallback);
                 }
             }

api/utils/common.js

@@ -463,6 +463,52 @@ common.recordAppKeyUsage = function(app, usedKey) {
     }
 };
 
+/**
+* Resolve an app by an incoming SDK app key for the ingestion/fetch hot path.
+* Looks up the accepted-keys array first, then falls back to the current-key
+* field. This is deliberately two single-field index lookups rather than one
+* {$or:[{key},{keys.key}]} query: MongoDB cannot reliably serve such an $or via
+* index union, so on this hot path it degrades to a full collection scan of the
+* apps collection. Each lookup here is an indexed equality, and the readBatcher
+* caches both (including misses), so the DB sees at most one query per shape per
+* cache period.
+*
+* The keys.key lookup matches every app once its accepted-keys array exists
+* (createApp/updateApp populate it, plus a one-time startup backfill), and also
+* matches a rotated-away old key during its grace period. The key fallback keeps
+* apps that predate the array — or that have not been backfilled yet — fully
+* resolvable, so correctness never depends on the backfill having run.
+* @param {string} appKey - incoming SDK app key
+* @param {function} callback - callback(err, app) with the resolved app or null
+* @returns {void}
+*/
+common.resolveAppByKey = function(appKey, callback) {
+    var key = appKey + "";
+    common.readBatcher.getOne("apps", {"keys.key": key}, function(err, app) {
+        if (app) {
+            return callback(err, app);
+        }
+        common.readBatcher.getOne("apps", {"key": key}, function(err2, app2) {
+            if (app2) {
+                return callback(err2, app2);
+            }
+            return callback(err2 || err, null);
+        });
+    });
+};
+
+/**
+* Invalidate any cached apps entry that resolveAppByKey may have loaded for an
+* incoming SDK key, covering both lookup shapes (keys.key and key).
+* @param {string} appKey - incoming SDK app key
+* @returns {void}
+*/
+common.invalidateAppByKey = function(appKey) {
+    var key = appKey + "";
+    common.readBatcher.invalidate("apps", {"keys.key": key}, {}, false);
+    common.readBatcher.invalidate("apps", {"key": key}, {}, false);
+};
+
 common.sha512Hash = function(str, addSalt) {
     var salt = (addSalt) ? new Date().getTime() : '';
     return crypto.createHmac('sha512', salt + '').update(str + '').digest('hex');

api/utils/requestProcessor.js

@@ -12,7 +12,7 @@ const Promise = require('bluebird');
 const url = require('url');
 const common = require('./common.js');
 const countlyCommon = require('../lib/countly.common.js');
-const { validateAppAdmin, validateUser, validateRead, validateUserForRead, validateUserForWrite, validateGlobalAdmin, dbUserHasAccessToCollection, validateUpdate, validateDelete, validateCreate, getBaseAppFilter } = require('./rights.js');
+const { validateAppAdmin, validateUser, validateRead, validateUserForRead, validateUserForWrite, validateGlobalAdmin, dbUserHasAccessToCollection, validateUpdate, validateDelete, validateCreate, getBaseAppFilter, getAdminApps, getUserAppsForFeaturePermission } = require('./rights.js');
 const authorize = require('./authorizer.js');
 const taskmanager = require('./taskmanager.js');
 const plugins = require('../../plugins/pluginManager.js');
@@ -1867,8 +1867,25 @@ const processRequest = (params) => {
                         params.qstring.query.subtask = {$exists: false};
                         params.qstring.query.app_id = params.qstring.app_id;
                         if (params.qstring.app_ids && params.qstring.app_ids !== "") {
-                            var ll = params.qstring.app_ids.split(",");
+                            var ll = params.qstring.app_ids.split(",").map(function(id) {
+                                return id.trim();
+                            }).filter(Boolean);
                             if (ll.length > 1) {
+                                // validateRead only checked the single app_id; every app
+                                // in the multi-app list must also be one the member can read
+                                if (!params.member.global_admin) {
+                                    var allowedTaskApps = (getAdminApps(params.member) || [])
+                                        .concat(getUserAppsForFeaturePermission(params.member, 'core', 'r') || []);
+                                    if (typeof params.member.permission === "undefined" && Array.isArray(params.member.user_of)) {
+                                        allowedTaskApps = allowedTaskApps.concat(params.member.user_of);
+                                    }
+                                    for (var taskAppIdx = 0; taskAppIdx < ll.length; taskAppIdx++) {
+                                        if (allowedTaskApps.indexOf(ll[taskAppIdx]) === -1) {
+                                            common.returnMessage(params, 401, 'User does not have access to one or more of the requested apps');
+                                            return;
+                                        }
+                                    }
+                                }
                                 params.qstring.query.app_id = {$in: ll};
                             }
                         }
@@ -2092,6 +2109,15 @@ const processRequest = (params) => {
                             common.returnMessage(params, 400, 'Missing parameter "collection"');
                             return false;
                         }
+                        // query params can be parsed into arrays/objects; force a
+                        // plain string before any substring check or access lookup
+                        params.qstring.collection = params.qstring.collection + "";
+                        // keep the db export surface aligned with DB Viewer: internal
+                        // index metadata and the dashboard session store are not exportable
+                        if (params.qstring.collection.indexOf("system.indexes") !== -1 || params.qstring.collection.indexOf("sessions_") !== -1) {
+                            common.returnMessage(params, 401, 'User does not have access right for this collection');
+                            return false;
+                        }
                         if (typeof params.qstring.filter === "string") {
                             try {
                                 params.qstring.query = JSON.parse(params.qstring.filter, common.reviver);
@@ -3517,7 +3543,7 @@ const validateAppForWriteAPI = (params, done, try_times) => {
         return done ? done() : false;
     }
 
-    common.readBatcher.getOne("apps", {$or: [{'key': params.qstring.app_key + ""}, {'keys.key': params.qstring.app_key + ""}]}, (err, app) => {
+    common.resolveAppByKey(params.qstring.app_key, (err, app) => {
         if (!app) {
             common.returnMessage(params, 400, 'App does not exist');
             params.cancelRequest = "App not found or no Database connection";
@@ -3566,10 +3592,10 @@ const validateAppForWriteAPI = (params, done, try_times) => {
                 if (err1) {
                     console.log("Failed to update apps collection " + err1);
                 }
-                //invalidate using the same query the request loaded the app
-                //with (current key OR an accepted old key), so the cache entry
-                //for old-key requests is also cleared
-                common.readBatcher.invalidate("apps", {$or: [{"key": params.qstring.app_key + ""}, {"keys.key": params.qstring.app_key + ""}]}, {}, false);
+                //invalidate both cache shapes the request may have loaded the
+                //app with (accepted-keys lookup, then current-key fallback), so
+                //the cache entry for old-key requests is also cleared
+                common.invalidateAppByKey(params.qstring.app_key);
             });
         }
 
@@ -3677,7 +3703,7 @@ const validateAppForFetchAPI = (params, done, try_times) => {
     if (ignorePossibleDevices(params)) {
         return done ? done() : false;
     }
-    common.readBatcher.getOne("apps", {$or: [{'key': params.qstring.app_key + ""}, {'keys.key': params.qstring.app_key + ""}]}, (err, app) => {
+    common.resolveAppByKey(params.qstring.app_key, (err, app) => {
         if (!app) {
             common.returnMessage(params, 400, 'App does not exist');
             params.cancelRequest = "App not found or no Database connection";

frontend/express/app.js

@@ -1981,6 +1981,33 @@ Promise.all([plugins.dbConnection(countlyConfig), plugins.dbConnection("countly_
     //startup (idempotent) so existing apps are covered after an upgrade with no
     //migration script; createApp also ensures it for new apps.
     countlyDb.collection('apps').createIndex({"keys.key": 1}, { background: true }, function() {});
+    //backfill the accepted-keys array for apps that predate key rotation, so the
+    //SDK app lookup resolves them via the indexed keys.key field on its first
+    //query instead of falling back to the current-key field. Idempotent (only
+    //touches apps missing the array, and is guarded again per-app at write time)
+    //and self-disabling once filled, so it is safe to run on every startup; this
+    //process runs in low replica count and the apps collection is small, so no
+    //migration script or cross-process coordination is needed. App resolution
+    //stays correct before this completes via the current-key fallback in
+    //common.resolveAppByKey, so it does not need to gate startup.
+    countlyDb.collection('apps').find({ keys: { $exists: false } }, { projection: { key: 1, created_at: 1, last_data: 1 } }).toArray(function(ferr, legacyApps) {
+        if (ferr) {
+            console.log("Failed to read apps for accepted-keys backfill", ferr);
+            return;
+        }
+        var nowSec = Math.floor(Date.now() / 1000);
+        (legacyApps || []).forEach(function(legacyApp) {
+            countlyDb.collection('apps').updateOne(
+                { _id: legacyApp._id, keys: { $exists: false } },
+                { $set: { keys: [{ key: legacyApp.key, added_at: legacyApp.created_at || nowSec, last_data: legacyApp.last_data || 0 }] } },
+                function(uerr) {
+                    if (uerr) {
+                        console.log("Failed to backfill accepted-keys array for app", legacyApp._id, uerr);
+                    }
+                }
+            );
+        });
+    });
     countlyDb.collection('members').createIndex({"api_key": 1}, { unique: true }, function() {});
     countlyDb.collection('members').createIndex({ email: 1 }, { unique: true }, function() {});
     countlyDb.collection('jobs').createIndex({ finished: 1 }, function() {});