diff --git a/plugins/compliance-hub/frontend/public/javascripts/countly.models.js b/plugins/compliance-hub/frontend/public/javascripts/countly.models.js index bd758822308..c4e6f858295 100644 --- a/plugins/compliance-hub/frontend/public/javascripts/countly.models.js +++ b/plugins/compliance-hub/frontend/public/javascripts/countly.models.js @@ -194,7 +194,13 @@ var ret = "

" + ((jQuery.i18n.map["systemlogs.action." + row.a]) ? jQuery.i18n.map["systemlogs.action." + row.a] : row.a) + "

"; if (typeof row.i === "object") { if (typeof row.i.app_id !== "undefined" && countlyGlobal.apps[row.i.app_id]) { - ret += "

" + jQuery.i18n.map["systemlogs.for-app"] + ": " + countlyGlobal.apps[row.i.app_id].name + "

"; + //this string is rendered with v-html, so every interpolated value must already be + //HTML-safe. Everything taken from "row" arrives through common.returnOutput, which + //escape_html_entities has already escaped, so it must NOT be escaped again here or + //the entities would show up literally. The app name is the exception: it comes from + //countlyGlobal, which is serialized into the dashboard's script island by + //express-expose and is never HTML-escaped, so it reaches us raw and is escaped here. + ret += "

" + jQuery.i18n.map["systemlogs.for-app"] + ": " + countlyCommon.encodeHtml(countlyGlobal.apps[row.i.app_id].name) + "

"; } if (typeof row.i.appuser_id !== "undefined") { ret += "

" + jQuery.i18n.map["systemlogs.for-appuser"] + ": " + row.i.appuser_id + "

"; diff --git a/plugins/populator/frontend/public/javascripts/countly.views.js b/plugins/populator/frontend/public/javascripts/countly.views.js index 8cea6458ca0..ec2b1655c56 100644 --- a/plugins/populator/frontend/public/javascripts/countly.views.js +++ b/plugins/populator/frontend/public/javascripts/countly.views.js @@ -552,7 +552,11 @@ saveButtonLabel: CV.i18n('common.yes'), cancelButtonLabel: CV.i18n('common.cancel'), title: CV.i18n('populator.environment-delete-warning-title'), - text: CV.i18n('populator.environment-delete-warning-description', this.filterByEnvironmentOptions.filter(x => x.value === this.environmentId)[0].label) + //this sentence is rendered with v-html because the localized string itself carries + //markup, so the environment name substituted into it has to be escaped here. The name + //was html-decoded when the dropdown options were built, which is what the dropdown + //needs, so the escaping the api applied no longer holds by this point. + text: CV.i18n('populator.environment-delete-warning-description', countlyCommon.encodeHtml(this.filterByEnvironmentOptions.filter(x => x.value === this.environmentId)[0].label)) }; }, calculateWidth: function(percentage) { diff --git a/plugins/populator/frontend/public/templates/populator.html b/plugins/populator/frontend/public/templates/populator.html index fcea9db058e..88aab7e6e71 100644 --- a/plugins/populator/frontend/public/templates/populator.html +++ b/plugins/populator/frontend/public/templates/populator.html @@ -170,7 +170,8 @@

diff --git a/test/unit-tests/plugins.compliance-hub.actions-escaping.js b/test/unit-tests/plugins.compliance-hub.actions-escaping.js new file mode 100644 index 00000000000..2c512055487 --- /dev/null +++ b/test/unit-tests/plugins.compliance-hub.actions-escaping.js @@ -0,0 +1,132 @@ +var should = require("should"); +var fs = require("fs"); +var path = require("path"); +var vm = require("vm"); + +// The export/purge history datatable builds an HTML string in onReady and the template renders it +// with v-html (plugins/compliance-hub/frontend/public/templates/exportHistory.html). Everything +// interpolated into that string therefore has to be HTML-safe already. +// +// Two opposite mistakes are possible and this file guards both directions: +// +// 1. NOT escaping the app name. It is read from countlyGlobal, which express-expose serializes +// into the dashboard's inline script island. That serializer escapes for the JavaScript string +// context only and is deliberately value-preserving, so the value arrives raw. An app admin can +// set their own app's name, and a global admin's dashboard lists every app, so an unescaped name +// is a stored cross-user XSS. +// 2. Escaping the values that came from the API. Those already went through +// common.escape_html_entities in common.returnOutput, so escaping them again would render the +// entities literally in the UI. +var SRC = path.resolve(__dirname, "../../plugins/compliance-hub/frontend/public/javascripts/countly.models.js"); + +/** + * Load countly.models.js in a sandbox and hand back the onReady callbacks it registers, + * keyed by data-table name. + * @param {object} apps - the countlyGlobal.apps map the module should see + * @returns {object} map of resource name to its onReady function + */ +function loadResources(apps) { + var resources = {}; + var noop = function() {}; + // matches countlyCommon.encodeHtml, which is `div.innerText = x; return div.innerHTML`: + // the text-node serializer escapes &, < and > and leaves quotes alone. + var encodeHtml = function(html) { + return (html + "").replace(/&/g, "&").replace(//g, ">"); + }; + var sandbox = { + window: {}, + countlyCommon: { + encodeHtml: encodeHtml, + formatTimeAgoText: function() { + return { text: "just now" }; + }, + getDescendantProp: noop, + API_PARTS: { data: { r: "/o" } }, + ACTIVE_APP_ID: "5f1a2b3c4d5e6f0011223344", + periodObj: {}, + getPeriodForAjax: function() { + return "30days"; + } + }, + CountlyHelpers: { createMetricModel: noop }, + jQuery: { i18n: { map: { "systemlogs.for-app": "For app", "systemlogs.for-appuser": "For app user", "systemlogs.action.export": "Data exported" } } }, + CV: { + i18n: function(k) { + return k; + } + }, + countlyGlobal: { apps: apps }, + countlyTaskManager: {}, + countlyVue: { + vuex: { + ServerDataTable: function(name, cfg) { + resources[name] = cfg.onReady; + return { name: name }; + }, + Module: function() { + return {}; + }, + MutationsFor: noop, + ActionsFor: noop + } + } + }; + sandbox.global = sandbox; + vm.createContext(sandbox); + vm.runInContext(fs.readFileSync(SRC, "utf8"), sandbox, { filename: SRC }); + return resources; +} + +describe("compliance-hub export history actions escaping", function() { + var APP_ID = "5f1a2b3c4d5e6f0011223344"; + + /** + * Run the export-history onReady over a single row. + * @param {string} appName - the app name countlyGlobal should carry + * @param {object} i - the row's "i" payload + * @returns {string} the built actions HTML + */ + function actionsFor(appName, i) { + var apps = {}; + apps[APP_ID] = { name: appName }; + var onReady = loadResources(apps).exportHistoryDataResource; + should.exist(onReady); + var rows = onReady({}, [{ a: "export", ts: 0, i: i || { app_id: APP_ID } }]); + return rows[0].actions; + } + + it("escapes an app name that carries a tag", function(done) { + var actions = actionsFor(''); + actions.indexOf(""); + actions.indexOf("").should.equal(-1); + done(); + }); + + it("leaves no raw angle bracket from the app name", function(done) { + var actions = actionsFor(""); + // the only markup left must be the

wrappers this builder emits itself + actions.replace(/<\/?p[^>]*>/g, "").indexOf("<").should.equal(-1); + done(); + }); + + it("keeps an ordinary app name readable", function(done) { + var actions = actionsFor("My Application"); + actions.should.containEql("For app: My Application"); + done(); + }); + + it("does not double-escape values the API already escaped", function(done) { + // returnOutput turns ' into '; escaping again would surface "&#39;" in the UI + var actions = actionsFor("My Application", { app_id: APP_ID, appuser_id: "user's-id" }); + actions.should.containEql("user's-id"); + actions.indexOf("&#39;").should.equal(-1); + done(); + }); +});