Skip to content

fix(reports): authorize non-core report targets on create and update - #7864

Open
ar2rsawseen wants to merge 4 commits into
masterfrom
security/reports-noncore-authorization
Open

fix(reports): authorize non-core report targets on create and update#7864
ar2rsawseen wants to merge 4 commits into
masterfrom
security/reports-noncore-authorization

Conversation

@ar2rsawseen

Copy link
Copy Markdown
Member

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 for this 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.

This restores that caller and gates both create and update on its result.

  • Update authorizes the merged stored-plus-payload report, not the payload alone, so a partial update cannot leave an unauthorized target in place and repointing an existing report at a different target is checked too.
  • A copy is passed, so the authorized flag — which is only how the dispatch returns its result — never reaches the written document.
  • Fails closed, matching the original intent of the commented-out code: a report type whose plugin does not answer /report/authorize is not authorized. Only "dashboards" implements it today, here and in countly-platform, so a plugin adding a new report type needs to implement the event for it.

Also drops validateCoreUser, which was commented out and superseded by the inline per-app check that uses the reports feature permission rather than plain app membership.

Tests

Real end-to-end coverage through the HTTP endpoints with a real non-admin member, so the whole path runs rather than the branch in isolation:

  • a member with reports rights but no view access to a private dashboard is refused when scheduling a report against it
  • the same member succeeds for a dashboard they own, which proves the authorize dispatch resolves rather than the request merely failing
  • the authorized flag does not appear on the stored report
  • a core report for an app the member has rights on still succeeds, so the restructuring did not change the core path

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

Propagation

Same change is going to release.24.05 and countly-platform main; both carry the identical commented-out caller.

🤖 Generated with Claude Code

ar2rsawseen and others added 2 commits July 30, 2026 00:59
/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 for this 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 both create and update on its result. Update
authorizes the merged stored-plus-payload report rather than the payload alone, so
a partial update cannot leave an unauthorized target in place, and repointing an
existing report at a different target is checked too. A copy is passed so the
authorize flag, which is only how the dispatch returns its result, never reaches
the written document.

Fails closed, matching the original intent of the commented-out code: a report
type whose plugin does not answer /report/authorize is not authorized. Only
"dashboards" implements it today, in this repo and in countly-platform, so a
plugin adding a new report type needs to implement the event for it.

Also drops validateCoreUser, which was commented out and superseded by the inline
per-app check that uses the reports feature permission rather than plain app
membership.

Co-Authored-By: Claude <noreply@anthropic.com>
Real end-to-end coverage through the HTTP endpoints with a real non-admin member,
so the whole path is exercised rather than the branch in isolation: validateCreate,
the report_type branch, the /report/authorize dispatch into the dashboards plugin,
and the insert.

- a member with reports rights but no view access to a private dashboard is
  refused when scheduling a report against it
- the same member succeeds for a dashboard they own, which proves the authorize
  dispatch resolves rather than the request simply failing
- the authorize flag does not appear on the stored report
- a core report for an app the member has rights on still succeeds, so the
  restructuring did not change the core path

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

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 29, 2026 22:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Dashboard permissions are deliberately separate from app permissions: a
dashboard can be shared with a member who has no access to the apps its widgets
reference, and they are meant to be able to view it and schedule a report for
it.

The suite proved a member is refused a dashboard they cannot view, and allowed
their own, but nothing covered the case in between, which is the one a
tightening change could plausibly break. This uses the same admin-owned
dashboard the member was already refused, changing only the share, so the test
isolates the share as the deciding factor rather than app rights.

Also generalised the cleanup to delete a list of dashboards rather than two
hard-coded ids.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 30, 2026 14:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Found while auditing for the same class as the alerts fix. The apps list was
authorized only when report_type was "core", which left a two step route onto any
app:

  create a report with report_type "dashboards" and apps naming the victim app.
  The apps check never ran, and the non-core check added earlier authorizes the
  dashboard target rather than the apps list, so pointing it at your own dashboard
  passed.

  update it with report_type "core" and no apps. effectiveType became "core", but
  the check only looked at the submitted apps, which was absent, so the stored
  list survived unchecked and the report started using it.

Then read it through preview or pdf, or wait for the scheduled mail. Precondition
was reports create and update on any single app, with no prior access to the
victim app at all.

apps is now authorized whenever a list is present, whatever the report type, and
on update the list that is authorized is the one the report will actually have:
the submitted list when one is sent, the stored list otherwise.

Safe for legitimate non-core reports because the dashboards drawer hides the app
picker, so those carry an empty apps, and an empty list stays permitted.

The shared check also keeps the legacy allowance the surrounding code already
makes, so members with no permission object are still authorized through user_of.

Tests walk both halves of the chain: create with an unauthorized apps list is
refused, and converting a stored report to core is refused when its stored apps
name an app the member cannot read.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 31, 2026 12:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ar2rsawseen

Copy link
Copy Markdown
Member Author

Scope widened: a second gap in the same handler

An audit for the class behind #7872 turned up a chain in this handler that the original fix here did not close, so it is fixed in this PR rather than a separate one.

The apps list was authorized only when report_type was "core", which left a two step route onto any app:

  1. Create with report_type: "dashboards" and apps: ["<victim>"]. The apps check never ran. The non-core check this PR originally added authorizes the dashboard target, not the apps list, so pointing it at your own dashboard passed.
  2. Update with {_id, report_type: "core"} and no apps. effectiveType became "core", but the check only looked at the submitted apps, which was absent, so the stored list survived unchecked and the report began using it.

Then read it through /i/reports/preview or /i/reports/pdf, or wait for the scheduled mail.

Precondition was reports create and update on any single app, with no prior access to the victim app. I verified both halves against this branch before fixing, so it was live even with the original change.

The fix

apps is authorized whenever a list is present, whatever the report type, and on update the list authorized is the one the report will actually end up with: the submitted list when one is sent, the stored list otherwise.

Why this does not break dashboards reports

The dashboards drawer sets showApps = false and hides the app picker, so a legitimate non-core report carries apps: []. An empty or absent list stays permitted, which is also what keeps report_type: "dashboards" reports for shared dashboards working, including the case where the member has no access to the apps whose widgets the dashboard contains.

The shared helper keeps the legacy allowance the surrounding code already makes, so members with no permission object remain authorized through user_of.

Tests

Two cases walking each half of the chain: create with an unauthorized apps list is refused, and converting a stored report to core is refused when its stored apps name an app the member cannot read. A second app the member has no rights on is created as a fixture and cleaned up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants