diff --git a/CHANGELOG.md b/CHANGELOG.md index 39d84eb7c7b..eea97813bf7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ Fixes: - [events] Fix sum chart tooltip displaying the raw floating-point value instead of a rounded number + +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 Features: - [journey_engine] dynamic parameter support in deeplinks diff --git a/plugins/dashboards/api/api.js b/plugins/dashboards/api/api.js index b1f3e5ec42f..85585c4081f 100644 --- a/plugins/dashboards/api/api.js +++ b/plugins/dashboards/api/api.js @@ -821,6 +821,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(); } @@ -1201,23 +1221,19 @@ plugins.setConfigs("dashboards", { widget.contenthtml = sanitizeNote(widget.contenthtml); } - //Filter out app_ids that current users does not have access to - if (widget.apps && Array.isArray(widget.apps)) { - var user_apps = getUserApps(params.member) || []; - var admin_apps = getAdminApps(params.member) || []; - widget.apps = widget.apps.filter(appId => { - if (params.member.global_admin) { - return true; - } - else if (user_apps && user_apps.indexOf(appId) !== -1) { - return true; - } - else if (admin_apps && admin_apps.indexOf(appId) !== -1) { - return true; - } - return false; - }); + //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) { @@ -1301,25 +1317,18 @@ plugins.setConfigs("dashboards", { common.returnMessage(params, 400, 'Invalid parameter: widget_id'); return true; } - //Filter out app_ids that current users does not have access to - if (widget.apps && Array.isArray(widget.apps)) { - var user_apps = getUserApps(params.member) || []; - var admin_apps = getAdminApps(params.member) || []; - widget.apps = widget.apps.filter(appId => { - if (params.member.global_admin) { - return true; - } - else if (user_apps && user_apps.indexOf(appId) !== -1) { - return true; - } - else if (admin_apps && admin_apps.indexOf(appId) !== -1) { - return true; - } - return false; - }); + //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."); @@ -1336,15 +1345,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) { @@ -1722,6 +1754,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(); + }); + }); + }); + }); + }); +});