chore(security): standardise the default response headers

CHANGELOG.md

@@ -8,6 +8,7 @@ Enterprise Fixes:
 - [data-manager] Fixed editing an event whose key contains `&` creating undeletable duplicate rows in the events table
 
 Security Fixes:
+- [core] Standardised the default security headers: dropped `X-XSS-Protection`, which is deprecated and removed from current browsers, dropped `preload` from the HSTS default so it stays an operator's choice, and added `Referrer-Policy`, `Permissions-Policy`, `Cross-Origin-Opener-Policy`. Routes that are embedded by SDKs (the ratings popup and its assets) drop the framing and cross-origin isolation headers as they already did for `X-Frame-Options`. An upgrade script brings existing installs in line while preserving any headers an operator added
 - [hooks] Internal event hooks are now scoped to the apps the hook belongs to: app creation is a global-admin-only event, and remote-config, cohort, alert and hook-chaining events are only delivered when the event's app is one the hook is scoped to
 - [compliance-hub] The consents table now returns a fixed set of fields; a projection supplied on the request is no longer used to widen the response beyond the consent columns
 - [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

api/api.js

@@ -121,8 +121,13 @@ plugins.connectToAllDatabases().then(function() {
         password_rotation: 3,
         password_autocomplete: true,
         robotstxt: "User-agent: *\nDisallow: /",
-        dashboard_additional_headers: "X-Frame-Options:deny\nX-XSS-Protection:1; mode=block\nStrict-Transport-Security:max-age=31536000; includeSubDomains; preload\nX-Content-Type-Options: nosniff",
-        api_additional_headers: "X-Frame-Options:deny\nX-XSS-Protection:1; mode=block\nStrict-Transport-Security:max-age=31536000; includeSubDomains; preload\nAccess-Control-Allow-Origin:*",
+        dashboard_additional_headers: "X-Frame-Options:deny\nStrict-Transport-Security:max-age=31536000; includeSubDomains\nX-Content-Type-Options: nosniff\nReferrer-Policy: strict-origin-when-cross-origin\nPermissions-Policy: camera=(), microphone=(), geolocation=(), payment=()\nCross-Origin-Opener-Policy: same-origin-allow-popups",
+        api_additional_headers: "X-Frame-Options:deny\nStrict-Transport-Security:max-age=31536000; includeSubDomains\nX-Content-Type-Options: nosniff\nReferrer-Policy: strict-origin-when-cross-origin\nPermissions-Policy: camera=(), microphone=(), geolocation=(), payment=()\nAccess-Control-Allow-Origin:*",
+        //Off by default: allow_access_control_origin is labelled as an
+        //Access-Control-Origin list only, so enforcing it as a framing rule could
+        //block an embed that works today. Turn on once the reported violations
+        //from a real deployment come back clean.
+        widget_frame_ancestors_enforce: false,
         dashboard_rate_limit_window: 60,
         dashboard_rate_limit_requests: 500,
         api_rate_limit_window: 0,

api/utils/common.js

@@ -1435,6 +1435,120 @@ common.returnRaw = function(params, returnCode, body, heads) {
     }
 };
 
+/**
+* Origin shapes accepted into a CSP source list: scheme, host and an optional
+* port, with nothing after it. Deliberately strict, because a browser discards
+* an entire directive that contains one source it cannot parse -- so a single
+* malformed entry in an app's list must not be able to void the whole policy.
+*/
+var FRAME_ANCESTOR_ORIGIN_REGEX = /^[a-zA-Z][a-zA-Z0-9+.-]*:\/\/(?:\[[0-9A-Fa-f:.]+\]|[a-zA-Z0-9._-]+)(?::\d{1,5})?$/;
+
+/**
+* Build the value of a CSP frame-ancestors directive out of an app's newline
+* separated allowed origin list, which is the same
+* app.plugins.allow_access_control_origin list that already drives
+* Access-Control-Allow-Origin. Entries that are not plain origins are skipped
+* rather than emitted.
+*
+* 'self' is always included, because the dashboard previews these same widget
+* routes in an iframe of its own and nobody lists their own Countly origin in
+* an allowed-origins list meant for their customer facing sites.
+* @param {String} allowedOrigins - newline separated list of exact origins
+* @returns {String|null} the directive value, or null when nothing usable is
+*                        configured, in which case the caller must omit the
+*                        header entirely instead of falling back to 'none'
+**/
+common.buildFrameAncestorsPolicy = function(allowedOrigins) {
+    if (typeof allowedOrigins !== "string") {
+        return null;
+    }
+    var entries = allowedOrigins.replace(/\r\n|\r|\n/g, "\n").split("\n");
+    var origins = [];
+    for (let i = 0; i < entries.length; i++) {
+        //a trailing slash is the one typo worth normalizing, since origins get
+        //copied out of a browser address bar; anything else has to already be
+        //an exact origin, the same way the CORS side requires
+        var origin = entries[i].trim().replace(/\/+$/, "");
+        if (FRAME_ANCESTOR_ORIGIN_REGEX.test(origin) && origins.indexOf(origin) === -1) {
+            origins.push(origin);
+        }
+    }
+    if (!origins.length) {
+        return null;
+    }
+    origins.unshift("'self'");
+    return "frame-ancestors " + origins.join(" ");
+};
+
+/**
+* Scope an SDK-embeddable widget response to the origins configured for its
+* app. X-Frame-Options is always removed: it cannot express a list of allowed
+* domains (ALLOW-FROM was never really implemented by browsers) and a browser
+* seeing both headers applies the stricter one, which would block the embed
+* regardless of the CSP.
+*
+* When the app has no usable origin configured the CSP header is left off, so
+* the many apps that never filled in the list keep embedding as before.
+*
+* Reported, not enforced, unless an operator opts in with
+* security.widget_frame_ancestors_enforce. allow_access_control_origin is
+* labelled purely as an Access-Control-Origin list and says nothing about
+* framing, so an app that listed only the origin its SDK makes XHR from would
+* have a working embed on another host blocked the moment this is enforced.
+* Report-only blocks nothing and surfaces those violations first.
+* @param {Object} res - express response object
+* @param {Object} app - app document, may be null when it could not be resolved
+**/
+common.setWidgetFrameHeaders = function(res, app) {
+    if (!res || typeof res.removeHeader !== "function") {
+        return;
+    }
+    res.removeHeader("X-Frame-Options");
+    var policy = common.buildFrameAncestorsPolicy(app && app.plugins && app.plugins.allow_access_control_origin);
+    if (!policy || typeof res.setHeader !== "function") {
+        return;
+    }
+    var enforce = false;
+    try {
+        enforce = !!(plugins.getConfig("security") || {}).widget_frame_ancestors_enforce;
+    }
+    catch (e) {
+        //no config loaded in this process: stay with the harmless default
+        enforce = false;
+    }
+    var headerName = enforce ? "Content-Security-Policy" : "Content-Security-Policy-Report-Only";
+    //an operator can already put a Content-Security-Policy into
+    //security.dashboard_additional_headers, so merge instead of overwriting and
+    //losing their directives; if they scoped framing themselves, leave it be
+    var existing = typeof res.getHeader === "function" ? res.getHeader(headerName) : null;
+    if (typeof existing === "string" && existing.trim().length) {
+        if (existing.indexOf("frame-ancestors") !== -1) {
+            return;
+        }
+        res.setHeader(headerName, existing.trim().replace(/;$/, "") + "; " + policy);
+        return;
+    }
+    res.setHeader(headerName, policy);
+};
+
+/**
+* Look up the app behind an embeddable widget request so its response can be
+* scoped to that app's configured origins. Best effort by design: any missing
+* or unexpected value resolves to null and the caller then omits the header.
+* @param {Object} db - countly database object
+* @param {String} appId - app id taken from the widget document
+* @param {Function} callback - called with the app document, or null
+* @returns {void} nothing, the result is handed to the callback
+**/
+common.getAppForWidget = function(db, appId, callback) {
+    if (!db || !appId || !/^[a-f0-9]{24}$/i.test(appId + "")) {
+        return callback(null);
+    }
+    db.collection("apps").findOne({_id: db.ObjectID(appId + "")}, {projection: {"plugins.allow_access_control_origin": 1}}, function(err, app) {
+        callback(err ? null : app || null);
+    });
+};
+
 common.returnMessage = function(params, returnCode, message, heads, noResult = false) {
     params.response = {
         code: returnCode,

bin/upgrade/DEV/scripts/standardize_security_headers.js

@@ -0,0 +1,113 @@
+/**
+ * Bring an existing install's configured security headers in line with the current
+ * defaults.
+ *
+ * Config defaults only seed keys that are absent: pluginManager.checkConfigs uses
+ * getObjectDiff, which copies a default across only when the stored value is undefined.
+ * So an install that already has security.dashboard_additional_headers keeps whatever it
+ * was first seeded with, forever, and a change to the shipped default reaches new
+ * installs only. That is why this script exists.
+ *
+ * It edits rather than overwrites, so anything an operator added by hand survives:
+ *
+ *  - drops X-XSS-Protection. Deprecated, removed from every current browser, and usable
+ *    as an attack primitive: it could be steered into disabling a page's own scripts,
+ *    and with mode=block the aborted load is observable cross-origin, which makes it an
+ *    oracle for reading page contents.
+ *  - drops the `preload` token from Strict-Transport-Security. Preload is a one-way
+ *    commitment that needs the domain submitted to the browser preload list and is
+ *    painful to undo, so it should be an operator's choice rather than a default. The
+ *    max-age and includeSubDomains parts are left alone.
+ *  - appends Referrer-Policy, Permissions-Policy and X-Content-Type-Options if absent.
+ *
+ * Deliberately does not add Cross-Origin-Opener-Policy or Cross-Origin-Resource-Policy:
+ * these headers are applied by global middleware that also covers the embeddable widget
+ * routes (/feedback/rating and the widget asset routes), which customers embed from
+ * their own origins, so setting them here would break those.
+ */
+
+const pluginManager = require('../../../../plugins/pluginManager.js');
+
+const KEYS = ['dashboard_additional_headers', 'api_additional_headers'];
+
+const REQUIRED = [
+    {name: 'X-Content-Type-Options', line: 'X-Content-Type-Options: nosniff'},
+    {name: 'Referrer-Policy', line: 'Referrer-Policy: strict-origin-when-cross-origin'},
+    {name: 'Permissions-Policy', line: 'Permissions-Policy: camera=(), microphone=(), geolocation=(), payment=()'}
+];
+
+/**
+ * Rewrite one configured header block.
+ * @param {string} value - the stored newline separated header string
+ * @returns {string|null} the new value, or null when nothing needed changing
+ */
+function rewrite(value) {
+    if (typeof value !== 'string') {
+        return null;
+    }
+    const lines = value.replace(/\r\n|\r/g, '\n').split('\n');
+    const kept = [];
+
+    for (const line of lines) {
+        const name = line.split(':')[0].trim().toLowerCase();
+        if (name === 'x-xss-protection') {
+            continue;
+        }
+        if (name === 'strict-transport-security') {
+            // remove only the preload token, keep max-age and includeSubDomains
+            kept.push(line.replace(/;\s*preload\b/i, ''));
+            continue;
+        }
+        kept.push(line);
+    }
+
+    const present = kept.map((l) => l.split(':')[0].trim().toLowerCase());
+    for (const req of REQUIRED) {
+        if (present.indexOf(req.name.toLowerCase()) === -1) {
+            kept.push(req.line);
+        }
+    }
+
+    const next = kept.filter((l) => l.trim().length).join('\n');
+    return next === value ? null : next;
+}
+
+pluginManager.dbConnection().then(async(db) => {
+    try {
+        const doc = await db.collection('plugins').findOne({_id: 'plugins'});
+        const security = (doc && doc.security) || {};
+        const update = {};
+
+        for (const key of KEYS) {
+            if (typeof security[key] === 'undefined') {
+                // never configured, so the shipped default applies already
+                console.log('security.' + key + ': not set, leaving to the default');
+                continue;
+            }
+            const next = rewrite(security[key]);
+            if (next === null) {
+                console.log('security.' + key + ': already current');
+                continue;
+            }
+            update['security.' + key] = next;
+            console.log('security.' + key + ': updating');
+            console.log('  from: ' + JSON.stringify(security[key]));
+            console.log('  to:   ' + JSON.stringify(next));
+        }
+
+        if (Object.keys(update).length) {
+            await db.collection('plugins').updateOne({_id: 'plugins'}, {$set: update});
+            console.log('Security headers updated');
+        }
+        else {
+            console.log('Nothing to update');
+        }
+    }
+    catch (err) {
+        console.error('Error while standardizing security headers', err);
+    }
+
+    db.close();
+});
+
+module.exports = {rewrite};

frontend/express/app.js

@@ -159,8 +159,13 @@ plugins.setConfigs("security", {
     password_rotation: 3,
     password_autocomplete: true,
     robotstxt: "User-agent: *\nDisallow: /",
-    dashboard_additional_headers: "X-Frame-Options:deny\nX-XSS-Protection:1; mode=block\nStrict-Transport-Security:max-age=31536000; includeSubDomains; preload\nX-Content-Type-Options: nosniff",
-    api_additional_headers: "X-Frame-Options:deny\nX-XSS-Protection:1; mode=block\nStrict-Transport-Security:max-age=31536000; includeSubDomains; preload\nAccess-Control-Allow-Origin:*",
+    dashboard_additional_headers: "X-Frame-Options:deny\nStrict-Transport-Security:max-age=31536000; includeSubDomains\nX-Content-Type-Options: nosniff\nReferrer-Policy: strict-origin-when-cross-origin\nPermissions-Policy: camera=(), microphone=(), geolocation=(), payment=()\nCross-Origin-Opener-Policy: same-origin-allow-popups",
+    api_additional_headers: "X-Frame-Options:deny\nStrict-Transport-Security:max-age=31536000; includeSubDomains\nX-Content-Type-Options: nosniff\nReferrer-Policy: strict-origin-when-cross-origin\nPermissions-Policy: camera=(), microphone=(), geolocation=(), payment=()\nAccess-Control-Allow-Origin:*",
+    //Off by default: allow_access_control_origin is labelled as an
+    //Access-Control-Origin list only, so enforcing it as a framing rule could
+    //block an embed that works today. Turn on once the reported violations
+    //from a real deployment come back clean.
+    widget_frame_ancestors_enforce: false,
     dashboard_rate_limit_window: 60,
     dashboard_rate_limit_requests: 500
 });

plugins/star-rating/frontend/app.js

@@ -22,7 +22,7 @@ var STAR_RATING_EXT_TO_MIME = {
 };
 
 (function(plugin) {
-    plugin.init = function(app) {
+    plugin.init = function(app, countlyDb) {
 
         var SAFE_PROVIDED_PATH_RE = /^[a-zA-Z0-9\-_./]*$/;
         /**
@@ -45,6 +45,27 @@ var STAR_RATING_EXT_TO_MIME = {
             return rawValue;
         }
 
+        /**
+         * Resolve the app that owns the requested widget, so the response can be
+         * scoped to the origins that app is allowed to be embedded on. Never
+         * fails the request: an unknown widget just yields no app, and the
+         * frame-ancestors header is then omitted.
+         * @param {*} widgetId - widget_id query parameter
+         * @param {Function} callback - called with the app document, or null
+         * @returns {void} nothing, the result is handed to the callback
+         */
+        function getWidgetApp(widgetId, callback) {
+            if (!countlyDb || typeof widgetId !== 'string' || !/^[a-f0-9]{24}$/i.test(widgetId)) {
+                return callback(null);
+            }
+            countlyDb.collection('feedback_widgets').findOne({_id: countlyDb.ObjectID(widgetId)}, {projection: {app_id: 1}}, function(err, widget) {
+                if (err || !widget) {
+                    return callback(null);
+                }
+                common.getAppForWidget(countlyDb, widget.app_id, callback);
+            });
+        }
+
         /**
          * Method that render ratings popup template
          * @param {*} req - Express request object
@@ -64,8 +85,21 @@ var STAR_RATING_EXT_TO_MIME = {
                 countlyPath = `/${countlyPath}`;
             }
 
+            //This popup is meant to be embedded by an SDK in a page on the customer's own
+            //origin, so the framing and cross-origin isolation headers the dashboard sets
+            //globally have to come back off for this response.
+            //
+            //Cross-Origin-Opener-Policy matters when the SDK opens the popup as a window
+            //rather than an iframe: it would otherwise sever window.opener and with it any
+            //callback the SDK expects.
             res.removeHeader('X-Frame-Options');
-            res.render('../../../plugins/star-rating/frontend/public/templates/feedback-popup', { countlyPath });
+            res.removeHeader('Cross-Origin-Opener-Policy');
+            //X-Frame-Options cannot name the origins allowed to frame this popup, so
+            //scope it with CSP frame-ancestors built from the app's own configuration
+            getWidgetApp(req.query.widget_id, function(widgetApp) {
+                common.setWidgetFrameHeaders(res, widgetApp);
+                res.render('../../../plugins/star-rating/frontend/public/templates/feedback-popup', { countlyPath });
+            });
         }
 
         app.get(countlyConfig.path + '/feedback/rating', renderPopup);

plugins/star-rating/tests.js

@@ -57,6 +57,26 @@ describe('Testing Rating plugin', function() {
                 });
         });
 
+        it('should not send X-Frame-Options and should not scope framing for an unresolvable widget', function(done) {
+            request.get('/feedback/rating')
+                .expect(200)
+                .end(function(err, res) {
+                    if (err) {
+                        return done(err);
+                    }
+                    //the widget must stay embeddable, so X-Frame-Options is gone
+                    should.not.exist(res.headers['x-frame-options']);
+                    //and with no app resolved there is nothing to scope framing to,
+                    //so no frame-ancestors may be emitted at all, enforced or reported
+                    ['content-security-policy', 'content-security-policy-report-only'].forEach(function(h) {
+                        if (res.headers[h]) {
+                            res.headers[h].should.not.containEql('frame-ancestors');
+                        }
+                    });
+                    done();
+                });
+        });
+
         it('should prefix every asset path with provided_url when it has a leading slash', function(done) {
             var providedUrl = '/reverse-proxy/countly';
             request.get('/feedback/rating?provided_url=' + encodeURIComponent(providedUrl))