[fix][hooks] keep app credentials out of the effect payload - #7941
Open
ar2rsawseen wants to merge 3 commits into
Open
[fix][hooks] keep app credentials out of the effect payload#7941ar2rsawseen wants to merge 3 commits into
ar2rsawseen wants to merge 3 commits 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.
ar2rsawseen
commented
Aug 24, 2026
… document comes first
/i/apps/update dispatches two app shaped objects, data.app and data.update, and the scrub
took the first branch that matched and stopped. data.app was cleaned; data.update was
handed to the effect pipeline whole.
data.update is not the safer of the two. api/parts/mgmt/apps.js rebuilds the accepted key
list on every update:
updatedApp.keys = keysArr;
so data.update.keys carries every key the app has ever had, on every single app update -
not only on a rotation. data.update.key is there whenever the key is rotated, and
data.update.id_key is filled in the first time an app that predates it is touched. So the
path this change exists to close was open on the most common app operation there is.
Both named fields are now scrubbed, and the "the payload is itself an app document" branch
is only taken when neither is present, since a payload carrying them is not itself an app.
Still keyed off field names and event type rather than by looking for a field called "key"
anywhere - "key" is an ordinary field elsewhere, events have one, and a blanket scrub would
break hooks that reference it.
The test fixture used update: {name: 'renamed'}, which is why this was not caught. It now
builds the update the way apps.js builds it, and there is a case asserting data.update is
cleaned while data.app sits beside it.
Test placement: the 24.05 branch has no plugins/hooks/tests directory, so the suite goes to
test/unit-tests there; the platform PR had no test for this at all and now carries it as a
.unit.js beside the plugin.
ar2rsawseen
added a commit
that referenced
this pull request
Aug 25, 2026
The four two-factor-auth cases I added were the only new failures in test-api-core on
this branch. They allowed 403 or 404, on the assumption that a build without the plugin
answers 404. It does not: requestProcessor answers 400 "Invalid path", so the cases
failed everywhere the plugin is not enabled - which is the CI test build.
Widening the allowed set to include 400 would have been the wrong repair. A case that
accepts 400, 403 and 404 passes whether or not the guard exists, which is worse than no
case at all. They now probe once for the route and skip with a reason when it is absent,
and assert exactly 403 when it is there.
Skipping leaves the guard unproven in this build, so the coverage that does not depend on
the plugin being installed is added beside it: a unit suite that reads the plugin source
and requires refuseScopedCredential at each method that mutates the factor, before the
write rather than after it, and requires the global-admin methods to keep using
validateUserForGlobalAdmin. Removing the guard from one arm fails it, checked by doing
exactly that. On 24.05 the generate-qr-code case reports pending, because that branch has
no such method.
For the record on the rest of this branch's test-api-core run: the other three failures
("user permission when app is deleted", "correct admins and users", "should return one
user") are not from this change. The same three fail on #7930, which carries none of it,
and test-api-core passed in the same hour on #7923, #7935, #7941, #7955, #7970, #7894 and
#7871. That is the flaky trio, not a regression.
ar2rsawseen
added a commit
that referenced
this pull request
Aug 25, 2026
The four two-factor-auth cases I added were the only new failures in test-api-core on
this branch. They allowed 403 or 404, on the assumption that a build without the plugin
answers 404. It does not: requestProcessor answers 400 "Invalid path", so the cases
failed everywhere the plugin is not enabled - which is the CI test build.
Widening the allowed set to include 400 would have been the wrong repair. A case that
accepts 400, 403 and 404 passes whether or not the guard exists, which is worse than no
case at all. They now probe once for the route and skip with a reason when it is absent,
and assert exactly 403 when it is there.
Skipping leaves the guard unproven in this build, so the coverage that does not depend on
the plugin being installed is added beside it: a unit suite that reads the plugin source
and requires refuseScopedCredential at each method that mutates the factor, before the
write rather than after it, and requires the global-admin methods to keep using
validateUserForGlobalAdmin. Removing the guard from one arm fails it, checked by doing
exactly that. On 24.05 the generate-qr-code case reports pending, because that branch has
no such method.
For the record on the rest of this branch's test-api-core run: the other three failures
("user permission when app is deleted", "correct admins and users", "should return one
user") are not from this change. The same three fail on #7930, which carries none of it,
and test-api-core passed in the same hour on #7923, #7935, #7941, #7955, #7970, #7894 and
#7871. That is the flaky trio, not a regression.
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
1 participant
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.
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.