fix(hooks): validate internal event trigger configuration on save

CHANGELOG.md

@@ -1,5 +1,6 @@
 ## Version 25.03.xx
 Fixes:
+- [hooks] Internal event hooks are validated on save: an unknown event type is rejected, and an event that names a cohort, hook or alert must name one belonging to the hook's own apps
 - [events] Fix sum chart tooltip displaying the raw floating-point value instead of a rounded number
 
 ## Version 25.03.50

plugins/hooks/api/api.js

@@ -347,6 +347,16 @@ plugins.register("/i/hook/save", function(ob) {
                         common.returnMessage(params, 403, "User does not have right");
                         return true;
                     }
+                    //validate against the effective hook, since an update may
+                    //change only the trigger or only the apps
+                    const updatedTriggerValidation = await validateTriggerConfiguration(
+                        hookConfig.trigger || existingHook.trigger,
+                        hookConfig.apps || existingHook.apps
+                    );
+                    if (!updatedTriggerValidation.valid) {
+                        common.returnMessage(params, 400, updatedTriggerValidation.error);
+                        return true;
+                    }
                     return common.db.collection("hooks").findAndModify(
                         { _id: common.db.ObjectID(id) },
                         {},
@@ -388,6 +398,12 @@ plugins.register("/i/hook/save", function(ob) {
                 common.returnMessage(params, 403, "User does not have right");
                 return true;
             }
+
+            const newTriggerValidation = await validateTriggerConfiguration(hookConfig.trigger, hookConfig.apps);
+            if (!newTriggerValidation.valid) {
+                common.returnMessage(params, 400, newTriggerValidation.error);
+                return true;
+            }
             return common.db.collection("hooks").insert(
                 hookConfig,
                 function(err, result) {
@@ -419,6 +435,101 @@ plugins.register("/i/hook/save", function(ob) {
     return true;
 });
 
+/**
+ * Validate an InternalEventTrigger's configuration against the hook's own apps.
+ *
+ * The event type itself was never checked, so any string was accepted and stored.
+ * More importantly, the events that name a target object matched on that id alone
+ * at delivery time, with nothing tying the target to the hook's apps: a hook
+ * scoped to one app could name another app's cohort, hook or alert. Delivery is
+ * now scoped, so this is a second line of defence, but it turns a hook that would
+ * silently never fire into an explicit rejection at save time.
+ *
+ * hooks and alerts are core collections, so a target that cannot be resolved is
+ * rejected. Cohorts are an enterprise plugin and the collection may not exist in
+ * a given deployment, so an unresolvable cohort is allowed through and left to the
+ * delivery-time check rather than blocking a working feature.
+ *
+ * @param {object} trigger - the effective trigger, payload merged over stored
+ * @param {array} apps - the effective app ids the hook is scoped to
+ * @returns {Promise<object>} {valid, error}
+ */
+async function validateTriggerConfiguration(trigger, apps) {
+    if (!trigger || trigger.type !== "InternalEventTrigger") {
+        return {valid: true};
+    }
+
+    const configuration = trigger.configuration || {};
+    const eventType = configuration.eventType;
+
+    if (!eventType || InternalEventTrigger.getInternalEvents().indexOf(eventType) === -1) {
+        return {valid: false, error: "Unknown internal event type"};
+    }
+
+    const scopedApps = Array.isArray(apps) ? apps.map(function(a) {
+        return a + "";
+    }) : [];
+
+    if (eventType === "/cohort/enter" || eventType === "/cohort/exit") {
+        if (!configuration.cohortID) {
+            return {valid: false, error: "Missing cohort for this event type"};
+        }
+        let cohort = null;
+        try {
+            //cohort ids are stored as strings, and the collection belongs to an
+            //enterprise plugin that may not be installed
+            cohort = await common.db.collection("cohorts").findOne({_id: configuration.cohortID + ""});
+        }
+        catch (e) {
+            cohort = null;
+        }
+        if (cohort && scopedApps.indexOf(cohort.app_id + "") === -1) {
+            return {valid: false, error: "Cohort does not belong to this hook's apps"};
+        }
+        return {valid: true};
+    }
+
+    if (eventType === "/hooks/trigger") {
+        if (!configuration.hookID) {
+            return {valid: false, error: "Missing hook for this event type"};
+        }
+        let sourceHook = null;
+        try {
+            sourceHook = await common.db.collection("hooks").findOne({_id: common.db.ObjectID(configuration.hookID + "")});
+        }
+        catch (e) {
+            sourceHook = null;
+        }
+        //the source hook's payload includes its whole document, so require it to be
+        //scoped within this hook's apps rather than merely overlapping
+        const sourceApps = (sourceHook && Array.isArray(sourceHook.apps)) ? sourceHook.apps : null;
+        if (!sourceApps || !sourceApps.length || !sourceApps.every(function(a) {
+            return scopedApps.indexOf(a + "") > -1;
+        })) {
+            return {valid: false, error: "Source hook does not belong to this hook's apps"};
+        }
+        return {valid: true};
+    }
+
+    if (eventType === "/alerts/trigger" && configuration.alertID) {
+        let alert = null;
+        try {
+            alert = await common.db.collection("alerts").findOne({_id: common.db.ObjectID(configuration.alertID + "")});
+        }
+        catch (e) {
+            alert = null;
+        }
+        const alertApps = (alert && Array.isArray(alert.selectedApps)) ? alert.selectedApps : [];
+        if (!alert || !alertApps.some(function(a) {
+            return scopedApps.indexOf(a + "") > -1;
+        })) {
+            return {valid: false, error: "Alert does not belong to this hook's apps"};
+        }
+    }
+
+    return {valid: true};
+}
+
 /***
  * @param {array} effects - array of effects
  */

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

@@ -310,6 +310,12 @@ class InternalEventTrigger {
 // hooks that subscribe to these global event types
 InternalEventTrigger.GLOBAL_EVENT_TYPES = GLOBAL_EVENT_TYPES;
 
+// exposed so the save handler can reject an eventType that is not a real
+// internal event, and can tell which events carry a target id to validate
+InternalEventTrigger.getInternalEvents = function() {
+    return InternalEvents.slice();
+};
+
 module.exports = InternalEventTrigger;
 const InternalEvents = [
     "/i/apps/create",

plugins/hooks/tests/index.js

@@ -1,4 +1,5 @@
 require('./crud.js');
 require('./authz.js');
+require('./trigger_config_authz.js');
 require('./email.js');
 require('./ssrf.js');

plugins/hooks/tests/trigger_config_authz.js

@@ -0,0 +1,174 @@
+var request = require('supertest');
+var should = require('should');
+var testUtils = require('../../../test/testUtils');
+request = request(testUtils.url);
+
+// Regression tests for validation of an InternalEventTrigger's configuration.
+//
+// The event type was never checked, so any string was accepted and stored. The
+// events that name a target object - cohortID, hookID, alertID - matched on that
+// id alone at delivery time, with nothing tying the target to the hook's own apps,
+// so a hook scoped to one app could name another app's object.
+//
+// Delivery is scoped now, so these rejections are a second line of defence. Their
+// practical value is that a hook which would silently never fire is refused at
+// save time with a reason, instead of being stored and quietly doing nothing.
+//
+// Deliberately not exempt for global admins: the delivery-side check is not
+// exempt either, so exempting save would let a global admin store a hook that can
+// never fire, which is the failure mode this is meant to remove.
+
+describe('Testing Hooks trigger configuration validation', function() {
+    var API_KEY_ADMIN = "";
+    var OWN_APP_ID = "";
+    var OTHER_APP_ID = "";
+    var foreignHookId = "";
+    var siblingHookId = "";
+    var createdHookIds = [];
+    var uniq = Date.now();
+
+    /**
+     * Build a hook config
+     * @param {string} name - hook name
+     * @param {array} apps - apps the hook is scoped to
+     * @param {object} triggerConfiguration - trigger configuration to use
+     * @returns {object} hook config
+     */
+    function hookFor(name, apps, triggerConfiguration) {
+        return {
+            name: name + "-" + uniq,
+            description: "trigger config validation",
+            apps: apps,
+            trigger: {type: "InternalEventTrigger", configuration: triggerConfiguration},
+            effects: [{type: "CustomCodeEffect", configuration: {code: "params.a = 1;"}}],
+            enabled: true
+        };
+    }
+
+    /**
+     * Save a hook
+     * @param {object} hookConfig - hook to save
+     * @param {number} expected - expected status code
+     * @returns {Promise<object>} supertest response
+     */
+    function saveHook(hookConfig, expected) {
+        return new Promise(function(resolve, reject) {
+            request.post('/i/hook/save?api_key=' + API_KEY_ADMIN + '&app_id=' + OWN_APP_ID)
+                .send({hook_config: JSON.stringify(hookConfig)})
+                .expect(expected)
+                .end(function(err, res) {
+                    if (err) {
+                        return reject(err);
+                    }
+                    resolve(res);
+                });
+        });
+    }
+
+    it('should set up a second app and a hook belonging only to it', async function() {
+        API_KEY_ADMIN = testUtils.get("API_KEY_ADMIN");
+        OWN_APP_ID = testUtils.get("APP_ID");
+
+        var appRes = await new Promise(function(resolve, reject) {
+            request.get('/i/apps/create?api_key=' + API_KEY_ADMIN + '&args=' + JSON.stringify({name: "HookCfgOther" + uniq}))
+                .expect(200)
+                .end(function(err, res) {
+                    return err ? reject(err) : resolve(res);
+                });
+        });
+        OTHER_APP_ID = appRes.body._id;
+        should.exist(OTHER_APP_ID);
+
+        var foreign = await saveHook(hookFor("foreign-source", [OTHER_APP_ID], {eventType: "/crashes/new"}), 200);
+        foreignHookId = foreign.body && (foreign.body._id || foreign.body);
+        should.exist(foreignHookId);
+        createdHookIds.push(foreignHookId);
+
+        var sibling = await saveHook(hookFor("sibling-source", [OWN_APP_ID], {eventType: "/crashes/new"}), 200);
+        siblingHookId = sibling.body && (sibling.body._id || sibling.body);
+        should.exist(siblingHookId);
+        createdHookIds.push(siblingHookId);
+    });
+
+    it('should reject an event type that is not a real internal event', async function() {
+        await saveHook(hookFor("bogus-event", [OWN_APP_ID], {eventType: "/i/not/an/event"}), 400);
+    });
+
+    it('should reject a hook chained to a hook belonging to another app', async function() {
+        await saveHook(hookFor("cross-app-chain", [OWN_APP_ID], {
+            eventType: "/hooks/trigger",
+            hookID: foreignHookId + ""
+        }), 400);
+    });
+
+    it('should reject a hook chain with no source hook named', async function() {
+        await saveHook(hookFor("chain-no-target", [OWN_APP_ID], {eventType: "/hooks/trigger"}), 400);
+    });
+
+    it('should allow a hook chained to a hook in the same app', async function() {
+        var res = await saveHook(hookFor("same-app-chain", [OWN_APP_ID], {
+            eventType: "/hooks/trigger",
+            hookID: siblingHookId + ""
+        }), 200);
+        var id = res.body && (res.body._id || res.body);
+        should.exist(id);
+        createdHookIds.push(id);
+    });
+
+    it('should allow an event type that carries no target id', async function() {
+        var res = await saveHook(hookFor("plain-event", [OWN_APP_ID], {eventType: "/i/app_users/create"}), 200);
+        var id = res.body && (res.body._id || res.body);
+        should.exist(id);
+        createdHookIds.push(id);
+    });
+
+    it('should reject retargeting an existing hook chain to another app hook', async function() {
+        var res = await saveHook(hookFor("retarget-me", [OWN_APP_ID], {
+            eventType: "/hooks/trigger",
+            hookID: siblingHookId + ""
+        }), 200);
+        var id = res.body && (res.body._id || res.body);
+        createdHookIds.push(id);
+
+        // update only the trigger, leaving apps to come from the stored hook
+        await new Promise(function(resolve, reject) {
+            request.post('/i/hook/save?api_key=' + API_KEY_ADMIN + '&app_id=' + OWN_APP_ID)
+                .send({
+                    hook_config: JSON.stringify({
+                        _id: id + "",
+                        trigger: {type: "InternalEventTrigger", configuration: {eventType: "/hooks/trigger", hookID: foreignHookId + ""}}
+                    })
+                })
+                .expect(400)
+                .end(function(err) {
+                    return err ? reject(err) : resolve();
+                });
+        });
+    });
+
+    after(function(done) {
+        var pending = createdHookIds.length;
+        /**
+         * Remove the second app once hooks are gone
+         * @returns {void}
+         */
+        function removeApp() {
+            request.get('/i/apps/delete?api_key=' + API_KEY_ADMIN + '&args=' + JSON.stringify({app_id: OTHER_APP_ID}))
+                .end(function() {
+                    done();
+                });
+        }
+        if (!pending) {
+            return removeApp();
+        }
+        createdHookIds.forEach(function(id) {
+            request.get('/i/hook/delete?api_key=' + API_KEY_ADMIN + '&app_id=' + OWN_APP_ID + '&hookID=' + id)
+                .end(function() {
+                    pending--;
+                    if (!pending) {
+                        removeApp();
+                    }
+                });
+        });
+    });
+});