security(alerts): authorize the apps an alert targets, not just who owns it

plugins/alerts/api/api.js

@@ -5,10 +5,12 @@ var Promise = require("bluebird");
 const JOB = require('../../../api/parts/jobs');
 const utils = require('./parts/utils.js');
 const _ = require('lodash');
-const { validateCreate, validateRead, validateUpdate, hasCreateRight, getAdminApps, getUserAppsForFeaturePermission } = require('../../../api/utils/rights.js');
+const { validateCreate, validateRead, validateUpdate, hasCreateRight, hasUpdateRight, getAdminApps, getUserAppsForFeaturePermission } = require('../../../api/utils/rights.js');
 const FEATURE_NAME = 'alerts';
 const commonLib = require("./parts/common-lib.js");
 const moment = require('moment-timezone');
+const { memberHasRightForAllApps } = require('./parts/app-authorization.js');
+
 
 /**
  * Alerts that can be triggered when an event is received.
@@ -248,22 +250,41 @@ function getScheduleTextExpression(period, offset) {
                     if (params.member.global_admin !== true) {
                         query.createdBy = params.member._id;
                     }
-                    return common.db.collection("alerts").findAndModify(
-                        query,
-                        {},
-                        {$set: alertConfig},
-                        function(err, result) {
-                            if (!err) {
-                                if (result && result.value) {
-                                    plugins.dispatch("/updateAlert", { method: "alertTrigger", alert: result.value });
+                    //The guard above only saw the submitted selectedApps, and an update
+                    //may leave that out to keep whatever is stored. So load the alert
+                    //and authorize the apps it currently targets before changing it,
+                    //otherwise an alert for an app the caller has since lost access to
+                    //stays editable through any app they do still hold.
+                    return common.db.collection("alerts").findOne({_id: common.db.ObjectID(id)}, function(findErr, existingAlert) {
+                        if (findErr) {
+                            common.returnMessage(params, 500, "Failed to save an alert");
+                            return;
+                        }
+                        if (existingAlert && params.member.global_admin !== true
+                            && !memberHasRightForAllApps(hasUpdateRight, params.member, existingAlert.selectedApps)) {
+                            log.d("Rejected alert update" + common.reqInfo(params) + ": alert " + id
+                                + " targets apps [" + (Array.isArray(existingAlert.selectedApps) ? existingAlert.selectedApps.join(", ") : "")
+                                + "] the caller may not update");
+                            common.returnMessage(params, 403, 'No alerts:update permission on the apps this alert targets');
+                            return;
+                        }
+                        common.db.collection("alerts").findAndModify(
+                            query,
+                            {},
+                            {$set: alertConfig},
+                            function(err, result) {
+                                if (!err) {
+                                    if (result && result.value) {
+                                        plugins.dispatch("/updateAlert", { method: "alertTrigger", alert: result.value });
+                                    }
+
+                                    common.returnOutput(params, result && result.value);
                                 }
-
-                                common.returnOutput(params, result && result.value);
-                            }
-                            else {
-                                common.returnMessage(params, 500, "Failed to save an alert");
-                            }
-                        });
+                                else {
+                                    common.returnMessage(params, 500, "Failed to save an alert");
+                                }
+                            });
+                    });
                 }
                 if (!alertConfig._id) {
                     alertConfig.createdAt = new Date().getTime();
@@ -317,6 +338,12 @@ function getScheduleTextExpression(period, offset) {
             try {
                 var query = { "_id": common.db.ObjectID(alertID) };
                 //If not global admin, limit delete to own alerts only
+                //
+                //Deliberately not also requiring rights on the apps the alert targets.
+                //Deleting only removes what the alert does, so it cannot be used to
+                //reach another app's data, and someone who has lost access to an app
+                //needs to remain able to remove the alert they own for it. Refusing here
+                //would leave them holding an alert they can neither manage nor delete.
                 if (params.member.global_admin !== true) {
                     query.createdBy = params.member._id;
                 }
@@ -366,7 +393,7 @@ function getScheduleTextExpression(period, offset) {
     plugins.register("/i/alert/status", function(ob) {
         let params = ob.params;
 
-        validateUpdate(params, FEATURE_NAME, function() {
+        validateUpdate(params, FEATURE_NAME, async function() {
             let statusList;
             try {
                 statusList = JSON.parse(params.qstring.status);
@@ -376,6 +403,50 @@ function getScheduleTextExpression(period, offset) {
                 common.returnMessage(params, 500, "Failed to change alert status" + err.message);
                 return;
             }
+            //Enabling an alert for an app the caller may no longer touch is what makes
+            //this endpoint exploitable, so the apps each stored alert targets are
+            //authorized before it is switched on.
+            //
+            //Switching one off is deliberately still allowed. It only reduces what the
+            //alert does, and refusing it would leave somebody who lost access to an app
+            //unable to stop alert mail they no longer want, which is worse than the
+            //problem being fixed.
+            const enablingIds = Object.keys(statusList).filter(function(alertID) {
+                return statusList[alertID] === true || statusList[alertID] === "true";
+            });
+            if (enablingIds.length > 0 && params.member.global_admin !== true) {
+                let toEnable = [];
+                try {
+                    //scoped to the caller's own alerts, the same as the update below.
+                    //An id belonging to somebody else must keep behaving as it did, a
+                    //silent no-op, rather than returning a 403 that would confirm an
+                    //alert with those apps exists.
+                    toEnable = await common.db.collection("alerts").find({
+                        _id: {
+                            $in: enablingIds.map(function(a) {
+                                return common.db.ObjectID(a);
+                            })
+                        },
+                        createdBy: params.member._id
+                    }, { projection: { selectedApps: 1 } }).toArray();
+                }
+                catch (e) {
+                    log.e("Failed to load alerts for a status change", e);
+                    common.returnMessage(params, 500, "Failed to change alert status");
+                    return;
+                }
+                const unauthorized = toEnable.filter(function(alert) {
+                    return !memberHasRightForAllApps(hasUpdateRight, params.member, alert.selectedApps);
+                });
+                if (unauthorized.length > 0) {
+                    log.d("Rejected alert status change" + common.reqInfo(params) + ": alert(s) "
+                        + unauthorized.map(function(a) {
+                            return a._id;
+                        }).join(", ") + " target apps the caller may not update");
+                    common.returnMessage(params, 403, 'No alerts:update permission on the apps these alerts target');
+                    return;
+                }
+            }
             const batch = [];
             for (const alertID in statusList) {
                 var qquery = { _id: common.db.ObjectID(alertID) };

plugins/alerts/api/jobs/monitor.js

@@ -4,7 +4,48 @@ const { TRIGGERED_BY_EVENT } = require('../parts/common-lib.js');
 
 const { Job } = require('../../../../api/parts/jobs/job.js'),
     log = require('../../../../api/utils/log.js')('alert:monitor'),
-    common = require('../../../../api/utils/common.js');
+    common = require('../../../../api/utils/common.js'),
+    { hasReadRight } = require('../../../../api/utils/rights.js');
+
+const { memberMayReadApp } = require('../parts/app-authorization.js');
+
+/**
+ * Whether the member who created an alert may still read the app it is about.
+ *
+ * The endpoints authorize a request, but nothing re-checked anything once an alert was
+ * enabled, so one created before access was revoked kept emailing that app's metrics
+ * indefinitely with no further action by anybody. This is the check that stops that, and
+ * it is why fixing only the write endpoints would not have been enough.
+ *
+ * @param {object} alert - the stored alert
+ * @param {string} appID - app the scheduled job is about
+ * @returns {Promise<boolean>} true when the alert may still be sent
+ */
+async function ownerMayStillReadApp(alert, appID) {
+    if (!alert.createdBy) {
+        //Alerts predating the createdBy field cannot be attributed to anybody. Refusing
+        //to send them would silently stop alerts that have worked for years, which is a
+        //worse outcome than the exposure closed here, so they are reported and left be.
+        log.w("Alert " + alert._id + " has no createdBy, so its owner's current access cannot be checked. Sending anyway.");
+        return true;
+    }
+    let owner;
+    try {
+        owner = await common.db.collection("members").findOne(
+            { _id: common.db.ObjectID(alert.createdBy + "") },
+            { projection: { global_admin: 1, permission: 1, user_of: 1, admin_of: 1 } }
+        );
+    }
+    catch (e) {
+        log.e("Could not resolve the owner of alert " + alert._id + ", not sending", e);
+        return false;
+    }
+    if (!owner) {
+        log.d("Owner of alert " + alert._id + " no longer exists, not sending");
+        return false;
+    }
+    return memberMayReadApp(hasReadRight, owner, appID);
+}
 
 const ALERT_MODULES = {
     "views": require("../alertModules/views.js"),
@@ -46,6 +87,10 @@ class MonitorJob extends Job {
             if (!alert || !app) {
                 throw new Error("Alert", alertID, "or App", appID, "couldn't be found");
             }
+            if (!await ownerMayStillReadApp(alert, appID)) {
+                log.d("Not sending alert " + alertID + " for app " + appID + ": its owner no longer has access to that app");
+                return;
+            }
             if (alert.alertDataType === 'profile_groups') {
                 alert.alertDataType = 'cohorts';
             }

plugins/alerts/api/parts/app-authorization.js

@@ -0,0 +1,95 @@
+/**
+ * @module plugins/alerts/api/parts/app-authorization
+ * @description Decides whether a member may act on the apps an alert targets.
+ *
+ * The alert endpoints authorize the caller against params.qstring.app_id, which says
+ * nothing about the apps the alert itself points at. Two further things have to be
+ * checked and were not:
+ *
+ *  - the apps the *stored* alert targets, on any mutation addressed by _id. The
+ *    submitted selectedApps is checked separately, but an update may leave that field
+ *    out and keep whatever is stored, so the stored value needs its own check.
+ *  - the owner's *current* access, at the moment an alert is sent. Nothing re-checked
+ *    that once an alert was enabled, so one created before access was revoked kept
+ *    emailing that app's metrics with no further action by anybody.
+ *
+ * createdBy is not a substitute for either. It says the member made the alert, not that
+ * they may still act on the apps it targets.
+ *
+ * This lives in its own module because the request path and the scheduled job both need
+ * it, including the legacy allowance below, and two copies of that would drift.
+ */
+
+'use strict';
+
+const FEATURE_NAME = 'alerts';
+
+/**
+ * Apps a member reaches only through the legacy fields.
+ *
+ * Members created before the permission object have none, and the right helpers then
+ * fall through to admin_of alone, so somebody who is merely user_of an app looks
+ * unauthorized. Without this they would lose the ability to manage their own alerts, and
+ * their alerts would stop being delivered. /o/alert/list makes the same allowance; all
+ * three have to agree or an alert could be listed and not editable, or editable and not
+ * delivered.
+ *
+ * @param {object} member - member object
+ * @returns {string[]} app ids granted by the legacy fields, empty for modern members
+ */
+function legacyApps(member) {
+    if (!member || typeof member.permission !== "undefined") {
+        return [];
+    }
+    return Array.isArray(member.user_of) ? member.user_of.map(String) : [];
+}
+
+/**
+ * Whether the member currently holds a right on every app in the list.
+ *
+ * An empty or missing list returns false: an alert that targets nothing is not something
+ * to authorize permissively.
+ *
+ * @param {function} rightFn - hasCreateRight / hasUpdateRight / hasReadRight
+ * @param {object} member - member object
+ * @param {Array} apps - app ids the alert targets
+ * @returns {boolean} true only when the member holds the right for every app
+ */
+function memberHasRightForAllApps(rightFn, member, apps) {
+    if (!member || !Array.isArray(apps) || apps.length === 0) {
+        return false;
+    }
+    if (member.global_admin) {
+        return true;
+    }
+    const legacy = legacyApps(member);
+    return apps.every(function(appId) {
+        return rightFn(FEATURE_NAME, appId + "", member) || legacy.indexOf(appId + "") > -1;
+    });
+}
+
+/**
+ * Whether the member may read one app's alerts. Used by the scheduled job, which deals
+ * with a single app rather than an alert's whole target list.
+ *
+ * @param {function} readRightFn - hasReadRight from rights.js
+ * @param {object} member - member object
+ * @param {string} appId - app id
+ * @returns {boolean} true when the member may read that app's alerts
+ */
+function memberMayReadApp(readRightFn, member, appId) {
+    if (!member) {
+        return false;
+    }
+    if (member.global_admin) {
+        return true;
+    }
+    return readRightFn(FEATURE_NAME, appId + "", member) || legacyApps(member).indexOf(appId + "") > -1;
+}
+
+module.exports = {
+    FEATURE_NAME,
+    legacyApps,
+    memberHasRightForAllApps,
+    memberMayReadApp
+};