security(reports): authorize the apps a report targets, on every path that uses them (24.05)

[ar2rsawseen]

Backport of #7864 to release.24.05. Applied cleanly; only the CHANGELOG heading needed resolving.

What changes

/i/reports/create and /i/reports/update authorized the target apps only when the effective report_type was "core". A non-core report_type names the plugin that owns the report, and those reports were inserted and updated without any authorization of the object they point at.

The authorization path already existed but was unreachable: the dashboards plugin implements /report/authorize, which checks view access to the dashboard a report renders, while the reports-side caller that invoked it (validateNonCoreUser) had been commented out.

Restores that caller and gates create and update on its result. Update authorizes the merged stored-plus-payload report, so a partial update cannot leave an unauthorized target in place and repointing an existing report is checked too. A copy is passed so the authorized flag never reaches the written document. Fails closed, matching the original intent: a report type whose plugin does not answer the event is not authorized, and only "dashboards" implements it today.

Also drops validateCoreUser, which was commented out and superseded by the inline per-app check.

Tests

Real end-to-end coverage through the HTTP endpoints with a real non-admin member: refused for a private dashboard they cannot view, allowed for one they own, the authorized flag absent from the stored report, and a core report still succeeding as a control.

Not executed locally, since the harness needs COUNTLY_TEST_API_KEY_ADMIN and COUNTLY_TEST_APP_ID. CI runs them in test-api-plugins.

Second commit: the paths that were still owner-only

recordUpdateOrDeleteQuery adds only user, so beyond create and update the remaining paths were scoped by ownership alone. Owning a report is not the same as still being allowed to read the apps it covers. A member who scheduled a report while they held an app, then lost that access while keeping reports rights elsewhere, could:

• re-enable it through /i/reports/status and let the schedule resume,
• mail themselves that app's figures on demand through /i/reports/send,
• read them straight out of the response through /i/reports/preview (and /i/reports/pdf where that endpoint exists). This is the most direct of them: no email involved, the figures come back in the HTTP response.

Nothing re-checked anything at send time either, so a report enabled before access was revoked kept mailing indefinitely. The scheduled job calls reports.getReport directly rather than sendReport, so both now consult a shared reports.ownerMayStillSend; guarding only sendReport would have missed the scheduled path entirely, which is the one that matters most.

Deliberately unchanged, with the reasoning recorded in the code:

• Disabling stays allowed. It only reduces what the report does, and refusing would leave someone unable to stop mail they no longer want.
• Delete stays owner-only. It cannot reach another app's data, and the owner needs a way to clean up.
• last_sent is not stamped when a report is skipped, so it resumes on its own if access is restored.

Reports with no apps pass, because those are non-core reports whose target is authorized by their own plugin through /report/authorize rather than per app.

A report whose owner no longer exists also stops being sent. A deleted account holds access to nothing, so the same rule applies to it; the getReport global-admin fallback stays where it is, because preview and pdf rely on it and both are authorized against the member making the request, but it no longer decides whether scheduled mail keeps going out. This matches alerts, which already stopped in that case.

Also drops a stray console.log of the status payload.

Worth a look separately

That global-admin fallback for an unresolvable owner quietly raises the access a report renders with. /i/users/delete removes a departing member's reports, so it should only affect orphaned rows, but I left it alone here rather than change send behaviour inside a security fix.

Tests for this commit

A suite that revokes a member's access to one app mid-test, covering both directions. The exploit: enable, send and preview are each refused, and the stored report is verified to still be disabled afterwards. The happy paths, which have to keep working: the member can still enable, disable, send and preview reports on the app they do hold, can still switch off and delete the revoked-app report, and a global admin is unaffected.

This branch has no /i/reports/pdf endpoint, so that guard from #7864 does not apply here. The inline allow-list the create and update paths built is extracted into a shared appsArePermitted, matching how master expresses it, rather than repeated three more times.

One test assertion differs from master deliberately: the preview happy path asserts "not 401" rather than a specific status, because this branch renders with if (params && params.res) { var html = res.message; ... } and lacks master's else if (res) guard, so a report for an app with no data can fail inside rendering. That is unrelated to authorization and is left alone.

🤖 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.