security(hooks): scope internal events to the hook's own apps - #7861
Open
ar2rsawseen wants to merge 5 commits into
Open
security(hooks): scope internal events to the hook's own apps#7861ar2rsawseen wants to merge 5 commits into
ar2rsawseen wants to merge 5 commits into
Conversation
The June global-event fix (59e4ec2, 53fefed) gated five instance-wide event types behind global-admin ownership, but the remaining InternalEventTrigger cases were left as they were. Several of them never consult rule.apps, so a hook created by a non-global member with hooks rights on one app received other apps' data. Rather than extend the allowlist again, every case now either appears in GLOBAL_EVENT_TYPES or checks the event's app id against the hook's apps. /i/apps/create is now a global event type. It cannot be app-scoped in any useful sense, because the app does not exist when the hook is configured, so no hook can legitimately name it. The payload is the new app document, which carries the SDK key, the immutable id_key, the accepted keys[] array and the checksum salt (checksum_salt is an accepted field on app create and nothing strips it before dispatch). It was the only /i/apps/* case that deliberately bypassed the app check. Hooks owned by a global admin still receive it. /i/remote-config/{add,update,remove}-{parameter,condition} are now delivered only when the event's app is one the hook is scoped to. Five of the six dispatch sites carried no app identifier at all, so they now include appId; it is copied onto the dispatched object rather than set on the stored document. A dispatch with no app id is dropped rather than broadcast. /cohort/enter and /cohort/exit matched on rule.trigger.configuration.cohortID alone. That value comes from the hook's own configuration and is not validated on save, and the handler then reads app_users<cohort.app_id> directly, so a hook scoped to one app could receive another app's cohort definition and full app_users documents. The cohort's app must now be one the hook is scoped to. /hooks/trigger matched on rule.trigger.configuration.hookID alone, equally unvalidated. fetchRules loads hooks with only error_logs excluded, so the forwarded ob.rule is the source hook's entire document, including every effect configuration (HTTP url and headers, custom code, email recipients) plus the data that fired it. The source hook must now be scoped within the subscribing hook's apps. /alerts/trigger fired every subscribed hook on the instance for any app's alert. Its dispatch carried no arguments at all, so it now passes the alert's app id and alert id, and delivery requires the app to be one the hook is scoped to. The effect payload stays empty as before; the new fields are used for scoping only and are not forwarded. The frontend keeps its own copy of the global-event list for hiding options from non-global admins, so /i/apps/create is added there too. Tests exercise process() directly, because delivery is driven by plugins.dispatch from other plugins and "the hook did not fire" is not observable over the API. Eight of the fourteen fail without this change; the six that pass either way are the positive controls, including that a global-admin-owned hook still receives /i/apps/create and that /crashes/new scoping is unaffected. Not changed here: the /i/apps/* and /i/app_users/* cases compare rule.apps[0] rather than testing membership, so a hook targeting two apps only ever fires for the first. That is a narrowness bug rather than a disclosure, and fixing it would start firing hooks that are currently silent, so it is left for a separate change. Co-Authored-By: Claude <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
The global-event case needs the owner lookup to resolve to a non-global-admin, and the first version of these tests did that by swapping out common.db for a stub. That is shared state for the whole test process, so replacing it even for a few milliseconds can break an unrelated suite with in-flight database work. A bad trade in a regression test, because an intermittent failure here is likely to get the test disabled rather than investigated. Uses a well-formed object id that is not a real member instead, so the real lookup returns nothing and the gate fails closed through its normal path. Drops the global-admin delivery case, which is the only one that needed the stub; the membership assertion on GLOBAL_EVENT_TYPES already covers that the event is gated, and that delivery path is shared with the five event types the earlier fix covered. Co-Authored-By: Claude <noreply@anthropic.com>
The existing scope tests call process() directly, which covers the scoping decision for every event family quickly and precisely but not the wiring around it: that the event is registered and dispatched at all, that fetchRules picks up a newly saved hook, and that the dispatch sites carry the app id the scoping now depends on. That last one matters most. Five of the six /i/remote-config/* dispatches previously sent no app identifier and were changed to include one. If that plumbing were wrong the scoping check would drop every event and the direct tests would still pass, because they feed the dispatch object in themselves. The failure mode is silent, since dropping is the fail-closed direction: hooks simply stop firing. So this drives a real remote-config change through the HTTP endpoints and observes the hook's own triggerCount. Both tests are kept: the direct ones for breadth, this one for the wiring. An HTTP probe server is not usable as the observable, because SSRF protection blocks localhost and 127.0.0.1 for effects, so the effect would never reach the probe even when delivery works and the test would pass for the wrong reason. Timing is the awkward part. fetchRules refreshes every 3s, the pipeline batches on 1s, and triggerCount goes through writeBatcher which flushes on batch_period, 10s by default, so delivery is observable about 11 seconds after the event. The positive case runs first and polls, which both proves delivery works and shows how long it takes; the negative case then waits that same full window before asserting nothing arrived, so it cannot pass merely by being read too early. Waits are scaled by testUtils.testScalingFactor. Co-Authored-By: Claude <noreply@anthropic.com>
Review catch. The assertions in this file all come from should extending Object.prototype, so the local binding was never read and CodeQL flagged it as an unused variable. Requiring the module without assigning keeps the extension in place and removes the unused local. Co-Authored-By: Claude <noreply@anthropic.com>
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.
Reported via
security@count.ly. Assessed against master321a6645591e21c6e41f9834614d56126b780f45.Context
The June global-event fix (
59e4ec2f9e5,53fefeddc0d) gated five instance-wide event types behind global-admin ownership viaGLOBAL_EVENT_TYPES. The remainingInternalEventTriggercases were left untouched, and several of them never consultrule.apps. A hook created by a non-global member withhooksrights on one app therefore received other apps' data.Rather than extend the allowlist a third time, every case now either appears in
GLOBAL_EVENT_TYPESor checks the event's app id against the hook's apps. That invariant is stated in the comment above the map so the next event added has to pick one.Per-event changes
/i/apps/createbecomes a global event type. It cannot be meaningfully app-scoped, because the app does not exist when the hook is configured, so no hook can legitimately name it. The payload is the new app document, carrying the SDKkey, the immutableid_key, the acceptedkeys[]array and the checksum salt (checksum_saltis an accepted field oncreateAppand nothing strips it before dispatch). It was the only/i/apps/*case that deliberately bypassed the sibling app check. Global-admin-owned hooks still receive it./i/remote-config/*(six events) are delivered only when the event's app is one the hook is scoped to. Five of the six dispatch sites carried no app identifier at all, so they now includeappId, copied onto the dispatched object rather than set on the stored document. A dispatch with no app id is dropped rather than broadcast./cohort/enterand/cohort/exitmatched onrule.trigger.configuration.cohortIDalone. That value comes from the hook's own configuration and is not validated on save, and the handler then readsapp_users<cohort.app_id>directly, so a hook scoped to one app could receive another app's cohort definition and full app-user documents. The cohort's app must now be one the hook is scoped to./hooks/triggermatched onrule.trigger.configuration.hookIDalone, equally unvalidated.fetchRulesloads hooks with onlyerror_logsexcluded, so the forwardedob.ruleis the source hook's entire document — every effect configuration, includingHTTPEffecturl and headers,CustomCodeEffectcode andEmailEffectrecipients — plus the data that fired it. The source hook must now be scoped within the subscribing hook's apps./alerts/triggerfired every subscribed hook on the instance for any app's alert. Its dispatch carried no arguments at all, so it now passes the alert's app id and alert id and requires the app to be one the hook is scoped to. The effect payload stays empty as before; the new fields are used for scoping only and are not forwarded.The frontend keeps its own copy of the global-event list for hiding options from non-global admins, so
/i/apps/createis added there too, with a note to keep the two in sync.Tests
plugins/hooks/tests/internal_event_scope.jsexercisesprocess()directly rather than over HTTP, because delivery is driven byplugins.dispatchfrom other plugins and "the hook did not fire" is not observable through the API.8 of the 14 fail without this change. The six that pass either way are the positive controls: a global-admin-owned hook still receives
/i/apps/create, same-app remote-config still delivers, same-app hook chaining still delivers, and/crashes/newscoping is unaffected.Not changed here
The
/i/apps/*and/i/app_users/*cases comparerule.apps[0]rather than testing membership, so a hook targeting two apps only ever fires for the first. That is a narrowness bug rather than a disclosure, and fixing it would start firing hooks that are currently silent, so it belongs in its own change.🤖 Generated with Claude Code