fix(hooks): validate internal event trigger configuration on save (24.05) - #7867
Open
ar2rsawseen wants to merge 1 commit into
Open
fix(hooks): validate internal event trigger configuration on save (24.05)#7867ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
/i/hook/save validated the hook's apps but never its trigger configuration. The event type was not checked at all, so any string was accepted and stored, and the events that name a target object matched on that id alone at delivery time with nothing tying the target to the hook's own apps. Adds validation on both create and update: - the event type must be one of the registered internal events - /hooks/trigger must name a source hook whose apps are within this hook's apps. Required to be a subset rather than merely overlapping, because the delivered payload includes the source hook's whole document - /cohort/enter and /cohort/exit must name a cohort belonging to one of the hook's apps - /alerts/trigger, when it names an alert, must name one belonging to one of the hook's apps. Not required to name one, since the field is optional today Update validates the effective hook, payload merged over stored, so changing only the trigger or only the apps is still checked, and retargeting an existing chain at another app's hook is rejected. hooks and alerts are core collections, so an unresolvable target is rejected. Cohorts belong to an enterprise plugin and the collection may be absent, so an unresolvable cohort is allowed through and left to the delivery-time check rather than blocking a working feature in a deployment where it cannot be verified. Deliberately not exempt for global admins. The delivery-side check is not exempt either, so exempting save would let a global admin store a hook that can never fire, which is the failure mode this removes: previously such a hook was accepted and then silently did nothing. Tests go through the real save endpoint and assert the rejections and the allowed cases, including the retarget-on-update path. 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.
Backport of #7866 to
release.24.05.What changes
/i/hook/savevalidated the hook'sappsbut never its trigger configuration. The event type was not checked at all, so any string was accepted and stored, and the events that name a target object matched on that id alone at delivery time, with nothing tying the target to the hook's own apps.Validation is added on both create and update:
/hooks/triggermust name a source hook whose apps are within this hook's apps. A subset rather than merely overlapping, because the delivered payload includes the source hook's whole document/cohort/enterand/cohort/exitmust name a cohort belonging to one of the hook's apps/alerts/trigger, when it names an alert, must name one belonging to one of the hook's apps. Not required to name one, since the field is optional todayUpdate validates the effective hook, payload merged over stored, so changing only the trigger or only the apps is still checked, and retargeting an existing chain at another app's hook is rejected.
hooksandalertsare core collections, so an unresolvable target is rejected. Cohorts belong to an enterprise plugin whose collection may be absent, so an unresolvable cohort is allowed through and left to the delivery-time check rather than blocking a feature that cannot be verified here.Not exempt for global admins, matching the delivery-side check. Exempting save would let a global admin store a hook that can never fire, which is the failure mode this removes.
Adapted for this branch
Two differences from master, both real rather than cosmetic:
findOnecallback are callback-style here rather thanasync, so both were madeasyncto host the validation. Master's handler is alreadyasync, so this brings the two in line rather than introducing a new pattern.plugins/hooks/tests.js, so the new describe block is appended there instead of added as a separate file.Tests
Through the real save endpoint: unknown event type rejected, cross-app chain rejected, chain with no source rejected, same-app chain allowed, an event carrying no target id allowed, and retargeting an existing chain at another app's hook rejected on update.
🤖 Generated with Claude Code