security(alerts): authorize the apps an alert targets, not just who owns it (24.05) - #7874
Open
ar2rsawseen wants to merge 1 commit into
Conversation
…tions 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 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Backports the alerts authorization fix to release.24.05 by ensuring alert mutations are authorized against the apps the stored alert targets (not just params.qstring.app_id), and by re-checking the owner’s current access at send time.
Changes:
- Introduces a shared alerts app-authorization helper (including legacy
user_ofallowance) and adds unit tests for it. - Tightens
/i/alert/save,/i/alert/status, and/i/alert/deleteto scope mutations to the alert owner and to validate targeted apps before enabling/updating. - Updates the monitor job to re-check the alert owner’s current read access before sending.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| test/unit-tests/plugins.alerts.app-authorization.js | Adds unit tests covering app authorization decisions (modern, legacy, global admin, multi-app targets). |
| plugins/alerts/api/parts/app-authorization.js | Adds shared authorization helpers for “all targeted apps” and “single app read” checks, including legacy handling. |
| plugins/alerts/api/jobs/monitor.js | Re-checks alert owner’s current read access before sending scheduled alerts. |
| plugins/alerts/api/api.js | Applies owner scoping + stored-target app authorization on update/enable paths; keeps delete owner-scoped. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //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) { |
| @@ -235,22 +236,47 @@ function getScheduleTextExpression(period, offset) { | |||
| const id = alertConfig._id; | |||
| delete alertConfig._id; | |||
| alertConfig.createdBy = params.member._id; | |||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #7872 to
release.24.05.Hand-authored rather than cherry-picked, because this branch had drifted in two ways.
The branch was missing the ownership scoping too
None of the three mutation paths filtered on
createdBy, so the earlier ownership fix was never backported here. Both that and the app authorization are in this PR.The vulnerability
The alert endpoints authorize the caller against
params.qstring.app_id, which says nothing about the apps the alert itself points at./i/alert/saveguards the submittedselectedApps, but an update may omit that field to keep whatever is stored, which skips the guard./i/alert/statushad no app check at all.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 by sending the request with an app they do still hold, and keep that app's metrics arriving at addresses of their choosing.
createdBydoes not help: it says who made the alert, not that they may still act on the apps it targets./i/alert/saveloads the alert and authorizes its storedselectedAppsbefore applying an update./i/alert/statusauthorizes 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/deletestays owner-only on purpose. Deleting cannot reach another app's data, and someone who lost access needs to remain able to clean up. The reasoning is recorded in the code so it does not read as an oversight.Two deliberate fail-open choices, to avoid breaking working installations: an alert with no
createdBypredates the field and cannot be attributed, so it is logged and still sent; and a status change for an id belonging to somebody else stays the silent no-op it already was, rather than a 403 that would confirm an alert with those apps exists.The shared decision lives in
plugins/alerts/api/parts/app-authorization.jsbecause the request path and the job both need it, including the legacyuser_ofallowance, and two copies would drift.Why the dependency on #7873 is real
hasAdminAccesson this branch answers "yes" for any app the member has no entry for, and the right helpers fall through to it. That makes this fix ineffective on its own: 4 of the 19 unit tests here fail against the unpatched helper, becausehasUpdateRight("alerts", "appB")returnstruefor an app the member holds nothing on.Measured on this branch:
release.24.05todayThe 5 (
validateArgs extra types,Countly Request) are pre-existing and unrelated; they fail identically on an unmodified checkout.Tests
19 unit tests over the authorization helper, covering both directions: the exploit shapes are refused, and the legacy
user_of/admin_ofpaths still resolve so those members keep managing and receiving their own alerts. Read access is checked not to grant update.eslint clean.
🤖 Generated with Claude Code