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

plugins/alerts/api/api.js

@@ -5,9 +5,10 @@ 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 { memberHasRightForAllApps } = require('./parts/app-authorization.js');
 const moment = require('moment-timezone');
 
 /**
@@ -235,22 +236,47 @@ function getScheduleTextExpression(period, offset) {
                     const id = alertConfig._id;
                     delete alertConfig._id;
                     alertConfig.createdBy = params.member._id;
-                    return common.db.collection("alerts").findAndModify(
-                        { _id: common.db.ObjectID(id) },
-                        {},
-                        {$set: alertConfig},
-                        function(err, result) {
-                            if (!err) {
-                                if (result && result.value) {
-                                    plugins.dispatch("/updateAlert", { method: "alertTrigger", alert: result.value });
+                    //Two things are missing on this branch and both are added here.
+                    //
+                    //The filter was the alert id alone, with no owner, so any member with
+                    //alerts rights on any app could rewrite any alert in the system. master
+                    //restricts this to the caller's own alerts and that never reached here.
+                    var query = { _id: common.db.ObjectID(id) };
+                    if (params.member.global_admin !== true) {
+                        query.createdBy = params.member._id;
+                    }
+                    //Owning an alert is not the same as being allowed to act on the apps it
+                    //targets, and the guard above only saw the submitted selectedApps, which
+                    //an update may leave out to keep whatever is stored. So authorize the
+                    //apps the stored alert currently points at.
+                    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: alert " + id + " targets apps the caller may not update");
+                            common.returnMessage(params, 403, 'No alerts:update permission on the apps this alert targets');
+                            return;
+                        }
+                        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");
+                                }
+                            });
+                    });
                 }
                 alertConfig.createdBy = params.member._id;
                 return common.db.collection("alerts").insert(
@@ -299,8 +325,20 @@ function getScheduleTextExpression(period, offset) {
         validateUpdate(params, FEATURE_NAME, function() {
             let alertID = params.qstring.alertID;
             try {
+                //the filter was the alert id alone, so any member with alerts rights on
+                //any app could delete any alert. master restricts this to the caller's own
+                //alerts and that never reached this branch.
+                //
+                //Deliberately not also requiring rights on the apps the alert targets:
+                //deleting only removes what the alert does, and somebody who has lost
+                //access to an app still needs to be able to remove the alert they own for
+                //it, or they are left holding one they can neither manage nor delete.
+                var deleteQuery = { "_id": common.db.ObjectID(alertID) };
+                if (params.member.global_admin !== true) {
+                    deleteQuery.createdBy = params.member._id;
+                }
                 common.db.collection("alerts").remove(
-                    { "_id": common.db.ObjectID(alertID) },
+                    deleteQuery,
                     function(err, result) {
                         log.d(err, result, "delete an alert");
                         if (!err) {
@@ -337,13 +375,56 @@ 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() {
             const statusList = JSON.parse(params.qstring.status);
+
+            //Enabling an alert for an app the caller may no longer touch is what makes
+            //this exploitable, so authorize the apps each stored alert targets before
+            //switching it on. Switching one off stays allowed: it only reduces what the
+            //alert does, and refusing would leave somebody who lost access unable to stop
+            //mail they no longer want.
+            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, as the update below is, so an id
+                    //belonging to somebody else stays a silent no-op rather than 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: alert(s) 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 appID in statusList) {
+                //the filter was the alert id alone, so any member could toggle any alert
+                var qquery = { _id: common.db.ObjectID(appID) };
+                if (params.member.global_admin !== true) {
+                    qquery.createdBy = params.member._id;
+                }
                 batch.push(
                     common.db.collection("alerts").findAndModify(
-                        { _id: common.db.ObjectID(appID) },
+                        qquery,
                         {},
                         { $set: { enabled: statusList[appID] } },
                         { new: true, upsert: false }

plugins/alerts/api/jobs/monitor.js

@@ -4,7 +4,46 @@ 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.
+ *
+ * @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 field cannot be attributed, and refusing to send them would
+        //silently stop alerts that have worked for years, 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"),
@@ -43,6 +82,10 @@ class MonitorJob extends Job {
                 _id: common.db.ObjectID(appID),
             });
             log.d("alert job info:", this._json, alert, app);
+            if (alert && app && !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 || !app) {
                 throw new Error("Alert", alertID, "or App", appID, "couldn't be found");
             }

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