[fix][hooks] keep app credentials out of the effect payload

plugins/hooks/api/parts/triggers/internal_event.js

@@ -59,6 +59,60 @@ async function isRuleOwnerGlobalAdmin(rule, cache) {
     return result;
 }
 
+//An app document carries credentials: the sdk key, every rotated key, the immutable
+//id_key, and the checksum salt. Effects can emit the payload verbatim, since the http
+//effect's body is a template and {{payload_json}} stringifies the whole thing to a url the
+//hook's author chose, so none of these belong in what hooks hands to an effect.
+//
+//This is done here rather than at the dispatch sites on purpose: systemlogs stores those
+//payloads whole so that a deleted or reset app can be recovered afterwards, and stripping
+//at the source would take the recoverable fields away with it. Everything below therefore
+//works on copies and never touches the object the other subscribers of the same dispatch
+//see.
+const APP_SECRET_FIELDS = ["key", "keys", "id_key", "salt", "checksum_salt"];
+
+/**
+ * Copy an app document without the fields that authenticate writes to it
+ * @param {object} app - app document from a dispatch payload
+ * @returns {object} copy without the credential fields
+ */
+function withoutAppSecrets(app) {
+    const copy = Object.assign({}, app);
+    APP_SECRET_FIELDS.forEach(function(field) {
+        delete copy[field];
+    });
+    return copy;
+}
+
+/**
+ * Copy trigger params with any app document's credentials removed. Keyed off the event
+ * type rather than by sniffing field names, because "key" is an ordinary field elsewhere:
+ * events have one, and scrubbing those would break hooks that reference it.
+ * @param {object} params - params about to be handed to the effect pipeline
+ * @param {string} eventType - internal event being processed
+ * @returns {object} params safe to hand onwards
+ */
+function withoutSecrets(params, eventType) {
+    if (!params || typeof params !== "object") {
+        return params;
+    }
+    const out = Object.assign({}, params);
+    if (out.data && typeof out.data === "object") {
+        //crashes/new and /i/apps/update nest the app document under data.app
+        if (out.data.app && typeof out.data.app === "object") {
+            out.data = Object.assign({}, out.data, {app: withoutAppSecrets(out.data.app)});
+        }
+        //while /i/apps/delete and /i/apps/reset pass the app document as data itself
+        else if (typeof eventType === "string" && eventType.indexOf("/i/apps/") === 0) {
+            out.data = withoutAppSecrets(out.data);
+        }
+    }
+    if (out.app && typeof out.app === "object") {
+        out.app = withoutAppSecrets(out.app);
+    }
+    return out;
+}
+
 /**
  * Internal event trigger
  */
@@ -73,6 +127,9 @@ class InternalEventTrigger {
         this.pipeline = () => {};
         if (options.pipeline) {
             this.pipeline = (data) => {
+                //before anything copies or forwards it, including the _originalInput
+                //snapshot kept for error records
+                data.params = withoutSecrets(data.params, data.eventType);
                 try {
                     data.rule._originalInput = JSON.parse(JSON.stringify(data.params || {}));
                 }
@@ -269,7 +326,7 @@ class InternalEventTrigger {
                     else if (!appId) {
                         warnMissingAppId("ob.appId");
                     }
-                    else if (rule.apps[0] === appId + '') {
+                    else if (Array.isArray(rule.apps) && rule.apps.indexOf(appId + '') > -1) {
                         utils.updateRuleTriggerTime(rule._id);
                         this.pipeline({
                             params: {data, appId, eventType},
@@ -295,10 +352,10 @@ class InternalEventTrigger {
                 if (!app_id) {
                     warnMissingAppId("ob.app_id");
                 }
-                else if (rule.apps[0] !== app_id + '') {
+                else if (!(Array.isArray(rule.apps) && rule.apps.indexOf(app_id + '') > -1)) {
                     noteOutOfScope(rule, app_id);
                 }
-                if (rule.apps[0] === app_id + '') {
+                if (Array.isArray(rule.apps) && rule.apps.indexOf(app_id + '') > -1) {
                     try {
                         utils.updateRuleTriggerTime(rule._id);
                     }
@@ -439,6 +496,9 @@ InternalEventTrigger.getInternalEvents = function() {
 };
 
 module.exports = InternalEventTrigger;
+//exported so the payload scrub can be unit tested without a live dispatch
+module.exports.withoutSecrets = withoutSecrets;
+
 const InternalEvents = [
     "/i/apps/create",
     "/i/apps/update",

plugins/hooks/tests/index.js

@@ -2,6 +2,7 @@ require('./crud.js');
 require('./authz.js');
 require('./internal_event_scope.js');
 require('./internal_event_delivery.js');
+require('./internal_event_payload.js');
 require('./trigger_config_authz.js');
 require('./email.js');
 require('./ssrf.js');

plugins/hooks/tests/internal_event_payload.js

@@ -0,0 +1,135 @@
+var should = require('should');
+var InternalEventTrigger = require('../api/parts/triggers/internal_event.js');
+
+var withoutSecrets = InternalEventTrigger.withoutSecrets;
+
+// An app document carries the sdk key, every rotated key, the immutable id_key and the
+// checksum salt. Effects can emit the payload verbatim, since the http effect's body is a
+// template and {{payload_json}} stringifies the whole thing to a url the hook's author
+// chose, so none of that may reach the effect pipeline.
+//
+// The dispatch payload itself must stay whole, because systemlogs records it so a deleted
+// or reset app can be recovered, so these cases also check the original is not mutated.
+var SECRET_FIELDS = ['key', 'keys', 'id_key', 'salt', 'checksum_salt'];
+
+/**
+ * Collect the paths of any credential field left anywhere in an object
+ * @param {object} obj - object to walk
+ * @returns {Array} dotted paths of the fields found
+ */
+function secretsIn(obj) {
+    var found = [];
+    /**
+     * Walk one level
+     * @param {object} o - current value
+     * @param {string} path - path so far
+     * @returns {void}
+     */
+    function walk(o, path) {
+        if (!o || typeof o !== 'object') {
+            return;
+        }
+        Object.keys(o).forEach(function(k) {
+            if (SECRET_FIELDS.indexOf(k) !== -1) {
+                found.push((path ? path + '.' : '') + k);
+            }
+            walk(o[k], (path ? path + '.' : '') + k);
+        });
+    }
+    walk(obj, '');
+    return found;
+}
+
+/**
+ * A representative app document
+ * @returns {object} app document with every credential field populated
+ */
+function appDoc() {
+    return {
+        _id: '6a41837e902bfd5369ddc610',
+        name: 'Test App',
+        timezone: 'UTC',
+        key: 'SDK_APP_KEY',
+        id_key: 'IMMUTABLE_KEY',
+        keys: [{key: 'SDK_APP_KEY', added_at: 1, last_data: 0}],
+        salt: 'CHECKSUM_SALT',
+        checksum_salt: 'CHECKSUM_SALT'
+    };
+}
+
+describe('Hooks internal event payload', function() {
+    describe('removes app credentials before the effect pipeline', function() {
+        var cases = [
+            // crashes/new and /i/apps/update nest the app under data.app
+            ['/crashes/new', function() {
+                return {data: {crash: {_id: 'c1'}, user: {uid: 'u1'}, app: appDoc()}, eventType: '/crashes/new'};
+            }],
+            ['/i/apps/update', function() {
+                return {data: {app: appDoc(), update: {name: 'renamed'}}, appId: appDoc()._id, eventType: '/i/apps/update'};
+            }],
+            // while delete and reset pass the app document as data itself
+            ['/i/apps/delete', function() {
+                return {data: appDoc(), appId: appDoc()._id, eventType: '/i/apps/delete'};
+            }],
+            ['/i/apps/reset', function() {
+                return {data: appDoc(), appId: appDoc()._id, eventType: '/i/apps/reset'};
+            }],
+            ['/i/apps/create', function() {
+                return {data: appDoc(), appId: appDoc()._id, eventType: '/i/apps/create'};
+            }]
+        ];
+
+        cases.forEach(function(entry) {
+            var label = entry[0];
+            var build = entry[1];
+            it('strips them from ' + label, function() {
+                var params = build();
+                should(secretsIn(params).length).be.above(0); // the fixture is representative
+                var out = withoutSecrets(params, params.eventType);
+                should(secretsIn(out)).eql([]);
+            });
+            it('leaves the dispatched payload itself intact for ' + label, function() {
+                var params = build();
+                var before = secretsIn(params).length;
+                withoutSecrets(params, params.eventType);
+                // other subscribers of the same dispatch, systemlogs in particular, still
+                // need the whole document
+                should(secretsIn(params).length).equal(before);
+            });
+        });
+
+        it('keeps the fields an effect actually uses', function() {
+            var out = withoutSecrets({data: appDoc(), appId: 'a1', eventType: '/i/apps/delete'}, '/i/apps/delete');
+            should(out.data).have.property('_id');
+            should(out.data).have.property('name', 'Test App');
+            should(out.data).have.property('timezone', 'UTC');
+            should(out).have.property('appId', 'a1');
+        });
+    });
+
+    describe('leaves unrelated payloads alone', function() {
+        // "key" is an ordinary field elsewhere: an event has one, so a blanket scrub would
+        // break any hook that references it
+        var untouched = [
+            ['an event with its own key', {data: {key: 'purchase', count: 1, sum: 9.99}, eventType: '/i/events'}],
+            ['incoming sdk data', {data: {events: [{key: 'login'}]}, eventType: '/sdk/data_ingestion'}],
+            ['an app user update', {data: {user: {uid: 'u1', custom: {key: 'value'}}}, eventType: '/i/app_users/update'}],
+            ['a cohort transition', {data: {cohort: {_id: 'co1', name: 'n'}, user: {uid: 'u1'}}, eventType: '/cohort/enter'}]
+        ];
+        untouched.forEach(function(entry) {
+            it('does not change ' + entry[0], function() {
+                var params = JSON.parse(JSON.stringify(entry[1]));
+                var out = withoutSecrets(params, params.eventType);
+                should(JSON.stringify(out)).equal(JSON.stringify(entry[1]));
+            });
+        });
+    });
+
+    describe('handles payloads that are not objects', function() {
+        it('returns them unchanged', function() {
+            should(withoutSecrets(undefined, '/i/apps/delete')).equal(undefined);
+            should(withoutSecrets(null, '/i/apps/delete')).equal(null);
+            should(withoutSecrets('a string', '/i/apps/delete')).equal('a string');
+        });
+    });
+});