[fix][hooks] keep app credentials out of the effect payload (24.05) - #7942
Open
ar2rsawseen wants to merge 1 commit into
Open
[fix][hooks] keep app credentials out of the effect payload (24.05)#7942ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #7941 to release.24.05.
No test here: this branch's hooks plugin has a single
plugins/hooks/tests.jsrather than thetests/directory the test on master plugs into.What
An app document carries the credentials that authenticate writes to that app:
key(the sdk key),keys[](every rotated key),id_key,saltandchecksum_salt. Four internal events carry such a document into the hooks pipeline:plugins/crashes/api/api.js/crashes/new{crash, user, app}api/parts/mgmt/apps.js/i/apps/update{app: appBefore, update}api/parts/mgmt/apps.js/i/apps/resetapi/parts/mgmt/apps.js/i/apps/deleteHooks 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.Change
Remove those fields where hooks takes the payload over: the single funnel every internal event passes through in
internal_event.js, before the_originalInputsnapshot copies it as well.Keyed off the event type rather than by field name, because
keyis an ordinary field elsewhere. An event has one, and a blanket scrub would break any hook that references it.Why here and not at the dispatch sites
Because
systemlogsrecords those payloads whole, deliberately, so that a deleted or reset app can be recovered afterwards. Stripping at the source would remove exactly the fields that recovery needs.The scrub therefore works on copies and never mutates the object the other subscribers of the same dispatch see. That is safe to rely on: across the three repositories these four events have 150 subscriber registrations, and none of them reads
key,keys,id_key,saltorchecksum_salt.appIdx31,datax4,paramsx3appIdx37,datax1appIdx72,datax5,paramsx3The
datareaders usedata._id,data.update.name,data.update,data.appfor a field diff, andtriggerByEvent(data)in alerts. All of that survives the scrub. Checked by extracting each handler body and resolving destructuring and aliases, not by a single line grep.Second change, correctness rather than security
Three checks compared
rule.apps[0]instead of testing membership, so a hook configured with several apps only fired for the first one. The neighbouring cohort and crash checks already useindexOf. Replaced with anindexOfmembership test. This one failed closed, so it never delivered across apps.Verification
plugins/hooks/tests/internal_event_payload.js, 16 cases, wired intoplugins/hooks/tests/index.jsso it actually runs: every app carrying event is stripped, the dispatched payload itself is left intact for each one, the fields an effect uses (_id,name,timezone,appId) survive, and four unrelated payloads that legitimately containkeyare unchanged. Replacing the scrub with a pass through fails 5 of them.node --checkclean on the changed files.