diff --git a/plugins/hooks/api/parts/triggers/internal_event.js b/plugins/hooks/api/parts/triggers/internal_event.js index 837bff12fc0..aae44355940 100644 --- a/plugins/hooks/api/parts/triggers/internal_event.js +++ b/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", diff --git a/plugins/hooks/tests/index.js b/plugins/hooks/tests/index.js index 8e1cec3484a..c5bca107da9 100644 --- a/plugins/hooks/tests/index.js +++ b/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'); diff --git a/plugins/hooks/tests/internal_event_payload.js b/plugins/hooks/tests/internal_event_payload.js new file mode 100644 index 00000000000..5f8e91fe420 --- /dev/null +++ b/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'); + }); + }); +});