From 6f81f6bb3832ed82791b248db03fdf0b9e9935c1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 20:46:18 +0300 Subject: [PATCH] security(dashboards): scope copied widgets to the copier's app access Backport of the master fix. This branch never received the January 2026 pentest fix (c27fd19e2d0), so it had no widget app filtering at all - dashboards/api.js imported only validateUser. On this branch the issue is therefore reachable without any sharing or copying: any authenticated member could create their own dashboard, add a widget with apps set to an arbitrary app id, and read that app's analytics through /o/dashboard/data. Dashboard permissions are intentionally separate from app permissions, so a dashboard may be shared with a user who cannot read the apps its widgets point at. Copying such a dashboard, however, made the copier the OWNER of the copied widgets, and owners have edit rights, letting a view-only recipient rewrite a borrowed widget's query and read more of that app than the dashboard owner chose to show them. Changes, matching master: - Copying a dashboard skips any widget referencing an app the copier cannot read. Widgets with no apps, such as notes, still copy. - add-widget validates the widget's app ids against the creator's access and rejects unauthorized ones instead of storing them. - Widget app lists are normalized to a real array of 24 character hex ids, so an array-like value such as {length: 1, "0": ""} cannot be stored and then read back through .length/[i] by fetchWidgetApps and the /dashboard/data handlers. - update-widget keeps the apps a widget already references, since the dashboard owner delegated that by granting edit access, but checks app ids being newly added against the editor's own access. Regression tests cover the reported copy chain, the array-like injection, adding a widget for an unreadable app, and that copying still works for a user who can read the apps. Co-Authored-By: Claude --- CHANGELOG.md | 3 + plugins/dashboards/api/api.js | 142 ++++++++++++++++++++++-- plugins/dashboards/tests.js | 200 ++++++++++++++++++++++++++++++++++ 3 files changed, 337 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 963b1750c10..7252721c071 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,7 @@ ## Version 24.05.51 +Security Fixes: +- [dashboards] Widgets are no longer copied when the copying user has no access to the apps they reference, and widget app ids are validated on widget create and update + Enterprise Fixes: - [data-manager] Fixed bug where event and view transformations occasionally failed to apply to incoming data diff --git a/plugins/dashboards/api/api.js b/plugins/dashboards/api/api.js index 1ca3133fe2e..d4a1fcd8a16 100644 --- a/plugins/dashboards/api/api.js +++ b/plugins/dashboards/api/api.js @@ -12,7 +12,7 @@ var pluginOb = {}, localize = require('../../../api/utils/localization.js'), async = require('async'), mail = require("../../../api/parts/mgmt/mail"), - { validateUser } = require('../../../api/utils/rights.js'); + { validateUser, getUserApps, getAdminApps } = require('../../../api/utils/rights.js'); var ejs = require("ejs"); @@ -807,6 +807,26 @@ plugins.setConfigs("dashboards", { function insertNewWidgets(dataObj, callback) { var widgets = dataObj.widgets || []; + //Copying a dashboard makes the copier the owner of the copied + //widgets, and owners have edit rights. A widget is therefore only + //copied when the copier can read every app it points at. + //Otherwise a view-only recipient could rewrite a borrowed + //widget's query and read more of that app than the dashboard + //owner chose to show them. Widgets that reference no apps, such + //as notes, are always copied. + widgets = widgets.filter(function(widget) { + if (typeof widget.apps === "undefined") { + return true; + } + + var widgetApps = normalizeAppIds(widget.apps); + if (widgetApps === null) { + return false; + } + + return memberHasAccessToAllApps(params.member, widgetApps); + }); + if (!widgets.length) { return callback(); } @@ -1181,6 +1201,20 @@ plugins.setConfigs("dashboards", { widget.contenthtml = sanitizeNote(widget.contenthtml); } + //A new widget may only reference apps the creator can read. Reject + //rather than silently strip, so a widget is never stored pointing at + //an app whose data will not be served. + var newWidgetApps = normalizeAppIds(widget.apps); + if (newWidgetApps === null) { + common.returnMessage(params, 400, 'Invalid parameter: widget.apps'); + return true; + } + if (!memberHasAccessToAllApps(params.member, newWidgetApps)) { + common.returnMessage(params, 403, 'Not allowed to use one or more of the given apps'); + return true; + } + widget.apps = newWidgetApps; + common.db.collection("dashboards").findOne({_id: common.db.ObjectID(dashboardId)}, function(err, dashboard) { if (err || !dashboard) { common.returnMessage(params, 400, "Dashboard with the given id doesn't exist"); @@ -1264,6 +1298,18 @@ plugins.setConfigs("dashboards", { return true; } + //Normalize the apps list up front so an array-like value such as + //{length: 1, "0": ""} cannot slip past the access check below + //while still being read through .length/[i] by every consumer. + if (typeof widget.apps !== "undefined") { + var updatedWidgetApps = normalizeAppIds(widget.apps); + if (updatedWidgetApps === null) { + common.returnMessage(params, 400, 'Invalid parameter: widget.apps'); + return true; + } + widget.apps = updatedWidgetApps; + } + common.db.collection("dashboards").findOne({_id: common.db.ObjectID(dashboardId), widgets: {$in: [common.db.ObjectID(widgetId)]}}, function(err, dashboard) { if (err || !dashboard) { common.returnMessage(params, 400, "Such dashboard and widget combination does not exist."); @@ -1280,15 +1326,38 @@ plugins.setConfigs("dashboards", { unsetQuery.$unset = {"isPluginWidget": ""}; } if (hasEditAccess) { - common.db.collection("widgets").findAndModify({_id: common.db.ObjectID(widgetId)}, {}, {$set: widget, ...unsetQuery }, {new: false}, function(er, result) { - if (er || !result || !result.value) { - common.returnMessage(params, 500, "Failed to update widget"); + //A member with edit access may keep the apps a widget + //already points at, even ones they cannot read - the + //dashboard owner delegated that by sharing with edit + //rights. They may not ADD an app they cannot read, + //which is what would turn a shared widget into a + //window onto an unrelated app. + common.db.collection("widgets").findOne({_id: common.db.ObjectID(widgetId)}, {projection: {apps: 1}}, function(readErr, existingWidget) { + if (readErr || !existingWidget) { + return common.returnMessage(params, 500, "Failed to update widget"); } - else { - plugins.dispatch("/systemlogs", {params: params, action: "widget_edited", data: {before: result.value, update: widget}}); - plugins.dispatch("/dashboard/widget/updated", {params: params, widget: {before: result.value, update: widget}}); - common.returnMessage(params, 200, 'Success'); + + if (typeof widget.apps !== "undefined") { + var existingApps = normalizeAppIds(existingWidget.apps) || []; + var addedApps = widget.apps.filter(function(appId) { + return existingApps.indexOf(appId) === -1; + }); + + if (!memberHasAccessToAllApps(params.member, addedApps)) { + return common.returnMessage(params, 403, 'Not allowed to use one or more of the given apps'); + } } + + common.db.collection("widgets").findAndModify({_id: common.db.ObjectID(widgetId)}, {}, {$set: widget, ...unsetQuery }, {new: false}, function(er, result) { + if (er || !result || !result.value) { + common.returnMessage(params, 500, "Failed to update widget"); + } + else { + plugins.dispatch("/systemlogs", {params: params, action: "widget_edited", data: {before: result.value, update: widget}}); + plugins.dispatch("/dashboard/widget/updated", {params: params, widget: {before: result.value, update: widget}}); + common.returnMessage(params, 200, 'Success'); + } + }); }); } else if (hasViewAccess) { @@ -1666,6 +1735,63 @@ plugins.setConfigs("dashboards", { return true; } + /** + * Function to normalize a widget apps value into an array of app id strings + * + * Only a genuine Array of 24 character hex strings is accepted. An + * array-like object such as {length: 1, "0": ""} is rejected here, + * because it would otherwise pass an Array.isArray() guard while still + * being consumed through .length/[i] by fetchWidgetApps and by every + * /dashboard/data handler. + * + * @param {Any} apps - widget apps value as supplied or as stored + * @returns {Array|null} array of app id strings, or null if not a valid list + */ + function normalizeAppIds(apps) { + if (!Array.isArray(apps)) { + return null; + } + + var appIds = []; + + for (var i = 0; i < apps.length; i++) { + var appId = apps[i]; + + if (typeof appId !== "string" || !/^[a-f0-9]{24}$/i.test(appId)) { + return null; + } + + if (appIds.indexOf(appId) === -1) { + appIds.push(appId); + } + } + + return appIds; + } + + /** + * Function to check whether a member can read every given app + * @param {Object} member - user + * @param {Array} appIds - normalized app id strings + * @returns {Boolean} true if the member has access to all of them + */ + function memberHasAccessToAllApps(member, appIds) { + if (member.global_admin) { + return true; + } + + var userApps = getUserApps(member) || []; + var adminApps = getAdminApps(member) || []; + + for (var i = 0; i < appIds.length; i++) { + if (userApps.indexOf(appIds[i]) === -1 && adminApps.indexOf(appIds[i]) === -1) { + return false; + } + } + + return true; + } + /** * Function to encode HTML * @param {String} html - HTML string diff --git a/plugins/dashboards/tests.js b/plugins/dashboards/tests.js index 0076b02d062..64f2710c367 100644 --- a/plugins/dashboards/tests.js +++ b/plugins/dashboards/tests.js @@ -91,3 +91,203 @@ describe('Testing dashboards widget-layout access control', function() { }); }); }); + +// Regression tests for cross-app widget scoping. +// +// Sharing a dashboard with someone who has no access to the apps its widgets +// point at is intentional - dashboard permissions are separate from app +// permissions. But copying such a dashboard makes the copier the OWNER of the +// copied widgets, and owners may edit them. A view-only recipient could +// therefore rewrite a borrowed widget's query (adding breakdowns, changing +// metrics) and read far more of an app than the dashboard owner chose to show. +// Widgets whose apps the copier cannot read must not be copied at all. + +describe('Testing dashboards cross-app widget scoping', function() { + var API_KEY_ADMIN = ""; + var APP_ID = ""; + var sourceDashId = ""; + var outsiderCopyId = ""; + var adminCopyId = ""; + var outsiderApiKey = ""; + var outsiderUserId = ""; + var uniq = Date.now(); + + it('should create a dashboard shared with all users', function(done) { + API_KEY_ADMIN = testUtils.get("API_KEY_ADMIN"); + APP_ID = testUtils.get("APP_ID"); + request + .get('/i/dashboards/create?api_key=' + API_KEY_ADMIN + '&name=SharedDash' + uniq + '&share_with=all-users') + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + sourceDashId = JSON.parse(res.text); + should.exist(sourceDashId); + done(); + }); + }); + + it('should add a widget for the test app as admin', function(done) { + var widget = { + widget_type: "number", + data_type: "session", + apps: [APP_ID], + metrics: ["u"], + title: "limited total users" + }; + request + .get('/i/dashboards/add-widget?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + sourceDashId + '&widget=' + encodeURIComponent(JSON.stringify(widget))) + .expect(200) + .end(function(err) { + if (err) { + return done(err); + } + done(); + }); + }); + + it('should show one widget on the source dashboard', function(done) { + request + .get('/o/dashboards/widget-layout?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + sourceDashId) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + JSON.parse(res.text).length.should.eql(1); + done(); + }); + }); + + it('should create a non-admin user with no app access', function(done) { + var userParams = { + full_name: "dashcopier" + uniq, + username: "dashcopier" + uniq, + password: "p4ssw0rD!", + email: "dashcopier" + uniq + "@mail.test", + permission: { _: { a: [], u: [] }, c: {}, r: {}, u: {}, d: {} } + }; + request + .get('/i/users/create?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify(userParams))) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + outsiderApiKey = res.body.api_key; + outsiderUserId = res.body._id; + should.exist(outsiderApiKey); + done(); + }); + }); + + it('should let the outsider copy the shared dashboard', function(done) { + request + .get('/i/dashboards/create?api_key=' + outsiderApiKey + '&name=CopiedDash' + uniq + '&share_with=none©_dash_id=' + sourceDashId) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + outsiderCopyId = JSON.parse(res.text); + should.exist(outsiderCopyId); + done(); + }); + }); + + it('should not copy widgets for apps the copier cannot read', function(done) { + request + .get('/o/dashboards/widget-layout?api_key=' + outsiderApiKey + '&dashboard_id=' + outsiderCopyId) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + var ob = JSON.parse(res.text); + Array.isArray(ob).should.eql(true); + ob.length.should.eql(0); + done(); + }); + }); + + it('should reject a widget for an app the user cannot read', function(done) { + var widget = { + widget_type: "number", + data_type: "session", + apps: [APP_ID], + metrics: ["u"] + }; + request + .get('/i/dashboards/add-widget?api_key=' + outsiderApiKey + '&dashboard_id=' + outsiderCopyId + '&widget=' + encodeURIComponent(JSON.stringify(widget))) + .expect(403) + .end(function(err) { + if (err) { + return done(err); + } + done(); + }); + }); + + it('should reject an array-like apps value', function(done) { + // {length: 1, "0": id} passes an Array.isArray() guard's falsy branch + // while still being read through .length/[i] downstream + var widget = { + widget_type: "number", + data_type: "session", + apps: {length: 1, "0": APP_ID}, + metrics: ["u"] + }; + request + .get('/i/dashboards/add-widget?api_key=' + outsiderApiKey + '&dashboard_id=' + outsiderCopyId + '&widget=' + encodeURIComponent(JSON.stringify(widget))) + .expect(400) + .end(function(err) { + if (err) { + return done(err); + } + done(); + }); + }); + + it('should still copy widgets for a copier who can read the apps', function(done) { + request + .get('/i/dashboards/create?api_key=' + API_KEY_ADMIN + '&name=AdminCopy' + uniq + '&share_with=none©_dash_id=' + sourceDashId) + .expect(200) + .end(function(err, res) { + if (err) { + return done(err); + } + adminCopyId = JSON.parse(res.text); + request + .get('/o/dashboards/widget-layout?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + adminCopyId) + .expect(200) + .end(function(e, r) { + if (e) { + return done(e); + } + JSON.parse(r.text).length.should.eql(1); + done(); + }); + }); + }); + + after(function(done) { + request + .get('/i/dashboards/delete?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + sourceDashId) + .end(function() { + request + .get('/i/dashboards/delete?api_key=' + API_KEY_ADMIN + '&dashboard_id=' + adminCopyId) + .end(function() { + request + .get('/i/dashboards/delete?api_key=' + outsiderApiKey + '&dashboard_id=' + outsiderCopyId) + .end(function() { + request + .get('/i/users/delete?api_key=' + API_KEY_ADMIN + '&args=' + encodeURIComponent(JSON.stringify({user_ids: [outsiderUserId]}))) + .end(function() { + done(); + }); + }); + }); + }); + }); +});