diff --git a/CHANGELOG.md b/CHANGELOG.md index 963b1750c10..f5a94120cef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,7 @@ ## Version 24.05.51 +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 + Enterprise Fixes: - [data-manager] Fixed bug where event and view transformations occasionally failed to apply to incoming data diff --git a/plugins/hooks/api/api.js b/plugins/hooks/api/api.js index c8c3e53918f..bd2d0712b90 100644 --- a/plugins/hooks/api/api.js +++ b/plugins/hooks/api/api.js @@ -295,7 +295,7 @@ plugins.register("/i/hook/save", function(ob) { let paramsInstance = ob.params; - validateCreate(ob.params, FEATURE_NAME, function(params) { + validateCreate(ob.params, FEATURE_NAME, async function(params) { let hookConfig = params.qstring.hook_config; try { hookConfig = JSON.parse(hookConfig); @@ -325,7 +325,7 @@ plugins.register("/i/hook/save", function(ob) { if (hookConfig._id) { const id = hookConfig._id; delete hookConfig._id; - return common.db.collection("hooks").findOne({ _id: common.db.ObjectID(id) }, function(findErr, existingHook) { + return common.db.collection("hooks").findOne({ _id: common.db.ObjectID(id) }, async function(findErr, existingHook) { if (findErr) { common.returnMessage(params, 500, "Failed to save an hook"); return; @@ -342,6 +342,16 @@ plugins.register("/i/hook/save", function(ob) { common.returnMessage(params, 403, "User does not have right"); return; } + //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; + } common.db.collection("hooks").findAndModify( { _id: common.db.ObjectID(id) }, {}, @@ -364,6 +374,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) { @@ -385,6 +401,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} {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 * @returns {boolean} isValid - true if all effects are valid diff --git a/plugins/hooks/api/parts/triggers/internal_event.js b/plugins/hooks/api/parts/triggers/internal_event.js index 77bcf8fcf46..3b0ecf22647 100644 --- a/plugins/hooks/api/parts/triggers/internal_event.js +++ b/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", diff --git a/plugins/hooks/tests.js b/plugins/hooks/tests.js index b3e0376bb45..836be00acec 100644 --- a/plugins/hooks/tests.js +++ b/plugins/hooks/tests.js @@ -232,3 +232,173 @@ describe('Testing Hooks', function() { }); + +// 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} 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(); + } + }); + }); + }); +});