From 6e547261dac3d75de59f99c29c4c9fbcad531e2a Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 14 Aug 2026 21:48:00 +0300 Subject: [PATCH] [fix][hooks] keep app credentials out of the effect payload An app document carries the credentials that authenticate writes to that app: the sdk key, every rotated key, the immutable id_key, and the checksum salt. Several internal events carry such a document, /crashes/new under data.app, /i/apps/update under data.app, and /i/apps/delete and /i/apps/reset as the payload itself. Hooks then hands the payload to its effects, and an effect can emit it verbatim: the http effect's body is a template and {{payload_json}} stringifies the whole payload to a url the hook's author chose. So a member who can create a hook on an app could have those fields posted to a host they control. Remove them where hooks takes the payload over, in the single funnel the internal event trigger passes everything through, before the _originalInput snapshot copies it too. Deliberately not at the dispatch sites: systemlogs records those payloads whole so that a deleted or reset app can be recovered afterwards, and stripping at the source would take the recoverable fields with it. The scrub therefore works on copies and leaves the object the other subscribers of the same dispatch see untouched. Across the three repositories 150 subscriber registrations read these events and none of them reads any of these fields. Keyed off the event type rather than by field name, because "key" is an ordinary field elsewhere: an event has one, and a blanket scrub would break hooks that reference it. Also fixes a hook with several apps only firing for the first of them: three checks compared rule.apps[0] instead of testing membership, while the neighbouring cohort and crash checks already use indexOf. That one fails closed, so it is a correctness fix rather than a security one. --- .../api/parts/triggers/internal_event.js | 66 ++++++++- plugins/hooks/tests/index.js | 1 + plugins/hooks/tests/internal_event_payload.js | 135 ++++++++++++++++++ 3 files changed, 199 insertions(+), 3 deletions(-) create mode 100644 plugins/hooks/tests/internal_event_payload.js 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'); + }); + }); +});