Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ar2rsawseen merged 1 commit into from
Jul 30, 2026
+337 −8
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Expand Down
142 changes: 134 additions & 8 deletions plugins/dashboards/api/api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");

Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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");
Expand Down Expand Up @@ -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": "<app id>"} 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.");
Expand All @@ -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) {
Expand Down Expand Up @@ -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": "<app id>"} 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
Expand Down
200 changes: 200 additions & 0 deletions plugins/dashboards/tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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&copy_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&copy_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();
});
});
});
});
});
});
Loading