From e7a6c61a95ce408d00a143103a8e5afec2eaf2e0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 20:07:39 +0300 Subject: [PATCH] security(alerts): authorize the apps an alert targets, and scope mutations to the owner Backport of the master fix (#7872) to release.24.05, hand-authored because this branch had drifted: none of the three mutation paths filtered on createdBy, so the earlier ownership scoping was never backported either. Both are here. The alert endpoints authorize the caller against params.qstring.app_id, which says nothing about the apps the alert itself points at, and an update may omit selectedApps to keep whatever is stored. So a member who created an alert for one app, then lost access to it while keeping alerts rights elsewhere, could still edit and re-enable that alert through an app they do still hold, and keep that app's metrics arriving at addresses of their choosing. createdBy says who made the alert, not that they may still act on the apps it targets. - /i/alert/save loads the alert and authorizes its stored selectedApps before applying an update - /i/alert/status authorizes before enabling. Disabling stays allowed, since it only reduces what the alert does and refusing would leave someone unable to stop mail they no longer want - /i/alert/delete stays owner-only on purpose, with the reasoning recorded - the monitor job re-checks the owner's current read access at send time, so an alert enabled before access was revoked stops delivering - all three mutation paths now scope to the caller's own alerts Depends on the hasAdminAccess fix for this branch (#7873). Without it the right helpers answer "yes" for apps the caller holds nothing on, which makes this fix ineffective: 4 of the 19 unit tests here fail against the unpatched helper. Co-Authored-By: Claude Opus 5 --- plugins/alerts/api/api.js | 119 +++++++++++++++--- plugins/alerts/api/jobs/monitor.js | 45 ++++++- plugins/alerts/api/parts/app-authorization.js | 95 ++++++++++++++ .../plugins.alerts.app-authorization.js | 109 ++++++++++++++++ 4 files changed, 348 insertions(+), 20 deletions(-) create mode 100644 plugins/alerts/api/parts/app-authorization.js create mode 100644 test/unit-tests/plugins.alerts.app-authorization.js diff --git a/plugins/alerts/api/api.js b/plugins/alerts/api/api.js index 6bf378e155e..96a2dc9d7a1 100644 --- a/plugins/alerts/api/api.js +++ b/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 } diff --git a/plugins/alerts/api/jobs/monitor.js b/plugins/alerts/api/jobs/monitor.js index fab1bf99b65..8aaf03b96d1 100644 --- a/plugins/alerts/api/jobs/monitor.js +++ b/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} 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"); } diff --git a/plugins/alerts/api/parts/app-authorization.js b/plugins/alerts/api/parts/app-authorization.js new file mode 100644 index 00000000000..02747b67b89 --- /dev/null +++ b/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 +}; diff --git a/test/unit-tests/plugins.alerts.app-authorization.js b/test/unit-tests/plugins.alerts.app-authorization.js new file mode 100644 index 00000000000..18e17db2fa8 --- /dev/null +++ b/test/unit-tests/plugins.alerts.app-authorization.js @@ -0,0 +1,109 @@ +require("should"); +var authz = require("../../plugins/alerts/api/parts/app-authorization.js"); +var rights = require("../../api/utils/rights.js"); + +// These decide whether a member may act on the apps an alert targets. Both directions +// matter equally: too permissive and an alert for a revoked app stays editable and keeps +// being delivered, too strict and members lose the ability to manage alerts they own. +// +// The legacy cases are the ones worth having tests for. Members created before the +// permission object reach apps through user_of, and the right helpers fall through to +// admin_of alone, so a strict reading would judge them unauthorized and both stop their +// alerts and lock them out of their own configuration. +describe("alerts app authorization", function() { + var modernMember = { + _id: "m1", + permission: { + _: {a: [], u: [["appA"]]}, + c: {appA: {all: false, allowed: {alerts: true}}}, + r: {appA: {all: false, allowed: {alerts: true}}}, + u: {appA: {all: false, allowed: {alerts: true}}}, + d: {} + } + }; + var legacyMember = {_id: "m2", user_of: ["appL"], admin_of: []}; + var legacyAdminMember = {_id: "m3", user_of: ["appL2"], admin_of: ["appL2"]}; + var globalAdmin = {_id: "m4", global_admin: true}; + + describe("legacyApps", function() { + it("returns nothing for a member with a permission object", function() { + authz.legacyApps(modernMember).should.eql([]); + }); + it("returns user_of for a member without one", function() { + authz.legacyApps(legacyMember).should.eql(["appL"]); + }); + it("tolerates a member with neither", function() { + authz.legacyApps({_id: "x"}).should.eql([]); + authz.legacyApps(null).should.eql([]); + }); + it("stringifies ids, since they may be stored as ObjectIds", function() { + var oid = { + toString: function() { + return "appZ"; + } + }; + authz.legacyApps({user_of: [oid]}).should.eql(["appZ"]); + }); + }); + + describe("memberHasRightForAllApps", function() { + it("allows an app the member holds the right on", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, ["appA"]).should.equal(true); + }); + it("refuses an app the member does not", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, ["appB"]).should.equal(false); + }); + it("requires every app, not just one of them", function() { + // an alert targeting two apps is only editable by someone who may touch both + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, ["appA", "appB"]).should.equal(false); + }); + it("allows a legacy member their user_of app", function() { + // without this a legacy member could not edit an alert they own + authz.memberHasRightForAllApps(rights.hasUpdateRight, legacyMember, ["appL"]).should.equal(true); + }); + it("refuses a legacy member an app outside user_of", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, legacyMember, ["appOther"]).should.equal(false); + }); + it("allows a legacy admin_of member", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, legacyAdminMember, ["appL2"]).should.equal(true); + }); + it("allows a global admin anything", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, globalAdmin, ["anything", "else"]).should.equal(true); + }); + it("refuses an empty or missing target list", function() { + // an alert that targets nothing is not something to wave through + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, []).should.equal(false); + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, undefined).should.equal(false); + authz.memberHasRightForAllApps(rights.hasUpdateRight, modernMember, null).should.equal(false); + }); + it("refuses a missing member", function() { + authz.memberHasRightForAllApps(rights.hasUpdateRight, null, ["appA"]).should.equal(false); + }); + it("distinguishes the rights, so read access does not grant update", function() { + var readOnly = { + _id: "m5", + permission: {_: {a: [], u: [["appR"]]}, c: {}, r: {appR: {all: false, allowed: {alerts: true}}}, u: {}, d: {}} + }; + authz.memberHasRightForAllApps(rights.hasReadRight, readOnly, ["appR"]).should.equal(true); + authz.memberHasRightForAllApps(rights.hasUpdateRight, readOnly, ["appR"]).should.equal(false); + }); + }); + + describe("memberMayReadApp", function() { + it("allows the app a member can read", function() { + authz.memberMayReadApp(rights.hasReadRight, modernMember, "appA").should.equal(true); + }); + it("refuses an app the member cannot read", function() { + authz.memberMayReadApp(rights.hasReadRight, modernMember, "appB").should.equal(false); + }); + it("allows a legacy member their user_of app, so their alerts keep arriving", function() { + authz.memberMayReadApp(rights.hasReadRight, legacyMember, "appL").should.equal(true); + }); + it("allows a global admin", function() { + authz.memberMayReadApp(rights.hasReadRight, globalAdmin, "whatever").should.equal(true); + }); + it("refuses a missing member, which is how a deleted owner looks", function() { + authz.memberMayReadApp(rights.hasReadRight, null, "appA").should.equal(false); + }); + }); +});