security(dashboards): scope copied widgets to the copier's app access

[ar2rsawseen]

Reported via security@count.ly. Assessed against master 321a6645591e21c6e41f9834614d56126b780f45.

Backport to release.24.05: #7857 (that branch is the more exposed of the two — see below).

The issue

Dashboard permissions are intentionally separate from app permissions, so a dashboard may be shared with a user who cannot read the apps its widgets reference. That part is by design.

Copying such a dashboard, however, made the copier the owner of the copied widgets, and owners have edit rights. A view-only recipient could copy a shared dashboard and then rewrite a borrowed widget's query — changing metrics, visualization and breakdowns while leaving the original app id in place — and read far more of that app than the dashboard owner chose to show them. /o/dashboard/data and the /dashboard/data handlers serve whatever is in widget.apps, so the rewritten widget returned data for an app the caller has no read access to.

The sharer granted a view of one thing; ownership transfer turned it into an arbitrary query.

What changes

• Copy is scoped to the copier. Copying a dashboard skips any widget referencing an app the copier cannot read. Widgets with no apps, such as notes, still copy.
• add-widget rejects rather than strips. Previously unauthorized app ids were silently filtered out, which stored a widget pointing at an app whose data would never be served. It now returns 403.
• Widget app lists are normalized. The existing filter was guarded by widget.apps && Array.isArray(widget.apps) while isWidgetValid only checked for undefined. A value that is array-like but not an Array therefore skipped the filter entirely, was stored as-is, and was still read through .length/[i] by fetchWidgetApps and by every /dashboard/data handler — allowing an arbitrary app id on a dashboard the user owns outright, with no sharing or copying involved. App lists are now normalized to a real array of 24-character hex ids and rejected otherwise.
• update-widget checks only newly added apps. A member with edit access keeps whatever apps the widget already references, since the dashboard owner delegated that by sharing with edit rights. App ids being newly added are checked against the editor's own access. This required reading the stored widget before the update, since the frontend round-trips the full widget object — a blanket check would have rejected legitimate edits to delegated widgets.

Why not enforce at read time instead

Filtering inside fetchWidgetApps was considered and rejected: it would break intentional cross-app dashboard sharing, which is a supported feature. It would also be insufficient on its own, because several /dashboard/data handlers read widget.apps directly rather than the resolved app map.

Tests

plugins/dashboards/tests.js gains four cases covering the copy chain, the array-like injection, add-widget for an unreadable app, and a positive case confirming copy still works for a user who can read the apps.

Verified locally: npx eslint plugins/dashboards/ and node --check pass. The plugin suite runs in CI (test-api-plugins).

Follow-ups, not in this PR

• Widgets are dropped silently on copy. A user-facing notice needs a change to the /i/dashboards/create response shape, which was kept out to keep this fix and its backport minimal.
• The copy_dash_id line in the /i/dashboards/update apidoc block is stale — that handler never reads it.
• The /report/authorize dispatch is commented out in plugins/reports/api/api.js, leaving /i/reports/create unauthorized for report_type: "dashboards". Bounded by the render-time owner login, but worth its own ticket.
• The dashboards plugin authorizes with getUserApps/getAdminApps (plain app membership) where alerts, hooks, reports and compare use getUserAppsForFeaturePermission. Aligning it is a separate change with regression risk.

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