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

[ar2rsawseen]

What

The alert endpoints authorize the caller against params.qstring.app_id, and the create path checks the submitted selectedApps. Neither says anything about the apps the stored alert points at, and an update may omit selectedApps to keep whatever is stored.

The by-id mutation paths filtered only on _id and createdBy. That is ownership, not authorization: a member who created an alert for one app and later 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.

/o/alert/list was already fixed for this and hides such alerts, so the write side was the remaining half.

Three changes

Update by id loads the alert and requires update rights on every app it targets, on top of the existing check on any newly submitted selectedApps.

The status endpoint requires the same before switching an alert 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, which is worse than the problem being fixed.

The scheduled job checks the owner still has read access before sending. 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 request from anybody. Fixing only the endpoints would have left that in place, and it is the more likely case in practice.

delete deliberately stays owner-only. It cannot reach another app's data, and the owner needs to be able to clean up an alert they can no longer manage.

The legacy trap this had to avoid

Members created before the permission object reach apps through user_of, and hasUpdateRight / hasReadRight then fall through to admin_of alone. A strict reading would judge them unauthorized, which would have locked them out of their own alerts and stopped delivery. /o/alert/list already makes that allowance, and now all three paths agree, otherwise an alert could be listed but not editable, or editable but not delivered.

The shared decision lives in parts/app-authorization.js because the request path and the scheduled job both need it. Two copies of that allowance would drift.

One deliberate fail-open

An alert with no resolvable createdBy is reported at warn level and still sent. Those predate the field, cannot be attributed, and refusing would silently stop alerts that have worked for years. That is a worse outcome than the exposure closed here, and it is visible in the log rather than silent.

Tests, both directions

• test/unit-tests/plugins.alerts.app-authorization.js, 19 cases: modern and legacy members, admin_of vs user_of, global admin, multi-app targets requiring all apps, empty target lists, missing members, and that read access does not confer update.
• plugins/alerts/tests.js, API tests through the real endpoints with a real member whose access is revoked mid-test.

The exploit cases: the update with selectedApps omitted is refused, enabling is refused, the alert is verified unchanged afterwards, and it stays hidden from the member's list.

The happy paths, which matter just as much: the member can still create, update, enable and disable their own alerts on the app they hold, can still switch off and delete the alert for the revoked app, and a global admin is unaffected.

Full unit suite: 204 passing. The 2 failures are Countly Request, which makes real HTTP calls and fails without network on a clean checkout too.

Note on scope

The status pre-check is scoped to the caller's own alerts, so an id belonging to somebody else keeps behaving as it did, a silent no-op, rather than returning a 403 that would confirm an alert with those apps exists.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.