From e5d6c7117135664ad34872ccd0b732fdf1c42edc Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Tue, 15 Jul 2025 11:25:25 -0400 Subject: [PATCH] Fixes #19633: Log all evaluations of invalid event rule conditions (#19885) * flush_events() should catch only import errors * Fixes #19633: Log all evaluations of invalid event rule conditions * Correct comment --- docs/configuration/system.md | 1 + netbox/extras/conditions.py | 23 ++++++++++++------- netbox/extras/events.py | 2 +- netbox/extras/models/models.py | 12 ++++++++-- netbox/extras/tests/test_conditions.py | 31 +++++++++++++++++--------- 5 files changed, 48 insertions(+), 21 deletions(-) diff --git a/docs/configuration/system.md b/docs/configuration/system.md index 20143276c..e294abb9c 100644 --- a/docs/configuration/system.md +++ b/docs/configuration/system.md @@ -158,6 +158,7 @@ LOGGING = { * `netbox..` - Generic form for model-specific log messages * `netbox.auth.*` - Authentication events * `netbox.api.views.*` - Views which handle business logic for the REST API +* `netbox.event_rules` - Event rules * `netbox.reports.*` - Report execution (`module.name`) * `netbox.scripts.*` - Custom script execution (`module.name`) * `netbox.views.*` - Views which handle business logic for the web UI diff --git a/netbox/extras/conditions.py b/netbox/extras/conditions.py index 5680be444..e1128c5dc 100644 --- a/netbox/extras/conditions.py +++ b/netbox/extras/conditions.py @@ -1,13 +1,14 @@ import functools +import operator import re from django.utils.translation import gettext as _ __all__ = ( 'Condition', 'ConditionSet', + 'InvalidCondition', ) - AND = 'and' OR = 'or' @@ -19,6 +20,10 @@ def is_ruleset(data): return type(data) is dict and len(data) == 1 and list(data.keys())[0] in (AND, OR) +class InvalidCondition(Exception): + pass + + class Condition: """ An individual conditional rule that evaluates a single attribute and its value. @@ -61,6 +66,7 @@ class Condition: self.attr = attr self.value = value + self.op = op self.eval_func = getattr(self, f'eval_{op}') self.negate = negate @@ -70,16 +76,17 @@ class Condition: """ def _get(obj, key): if isinstance(obj, list): - return [dict.get(i, key) for i in obj] - - return dict.get(obj, key) + return [operator.getitem(item or {}, key) for item in obj] + return operator.getitem(obj or {}, key) try: value = functools.reduce(_get, self.attr.split('.'), data) - except TypeError: - # Invalid key path - value = None - result = self.eval_func(value) + except KeyError: + raise InvalidCondition(f"Invalid key path: {self.attr}") + try: + result = self.eval_func(value) + except TypeError as e: + raise InvalidCondition(f"Invalid data type at '{self.attr}' for '{self.op}' evaluation: {e}") if self.negate: return not result diff --git a/netbox/extras/events.py b/netbox/extras/events.py index 95170e18d..d7c642c4e 100644 --- a/netbox/extras/events.py +++ b/netbox/extras/events.py @@ -192,5 +192,5 @@ def flush_events(events): try: func = import_string(name) func(events) - except Exception as e: + except ImportError as e: logger.error(_("Cannot import events pipeline {name} error: {error}").format(name=name, error=e)) diff --git a/netbox/extras/models/models.py b/netbox/extras/models/models.py index 9da2a8d9e..aa5af892f 100644 --- a/netbox/extras/models/models.py +++ b/netbox/extras/models/models.py @@ -13,7 +13,7 @@ from rest_framework.utils.encoders import JSONEncoder from core.models import ObjectType from extras.choices import * -from extras.conditions import ConditionSet +from extras.conditions import ConditionSet, InvalidCondition from extras.constants import * from extras.utils import image_upload from extras.models.mixins import RenderTemplateMixin @@ -142,7 +142,15 @@ class EventRule(CustomFieldsMixin, ExportTemplatesMixin, TagsMixin, ChangeLogged if not self.conditions: return True - return ConditionSet(self.conditions).eval(data) + logger = logging.getLogger('netbox.event_rules') + + try: + result = ConditionSet(self.conditions).eval(data) + logger.debug(f'{self.name}: Evaluated as {result}') + return result + except InvalidCondition as e: + logger.error(f"{self.name}: Evaluation failed. {e}") + return False class Webhook(CustomFieldsMixin, ExportTemplatesMixin, TagsMixin, ChangeLoggedModel): diff --git a/netbox/extras/tests/test_conditions.py b/netbox/extras/tests/test_conditions.py index dfe460f99..236c53eaa 100644 --- a/netbox/extras/tests/test_conditions.py +++ b/netbox/extras/tests/test_conditions.py @@ -4,7 +4,7 @@ from django.test import TestCase from core.events import * from dcim.choices import SiteStatusChoices from dcim.models import Site -from extras.conditions import Condition, ConditionSet +from extras.conditions import Condition, ConditionSet, InvalidCondition from extras.events import serialize_for_event from extras.forms import EventRuleForm from extras.models import EventRule, Webhook @@ -12,16 +12,11 @@ from extras.models import EventRule, Webhook class ConditionTestCase(TestCase): - def test_dotted_path_access(self): - c = Condition('a.b.c', 1, 'eq') - self.assertTrue(c.eval({'a': {'b': {'c': 1}}})) - self.assertFalse(c.eval({'a': {'b': {'c': 2}}})) - self.assertFalse(c.eval({'a': {'b': {'x': 1}}})) - def test_undefined_attr(self): c = Condition('x', 1, 'eq') - self.assertFalse(c.eval({})) self.assertTrue(c.eval({'x': 1})) + with self.assertRaises(InvalidCondition): + c.eval({}) # # Validation tests @@ -37,10 +32,13 @@ class ConditionTestCase(TestCase): # dict type is unsupported Condition('x', 1, dict()) - def test_invalid_op_type(self): + def test_invalid_op_types(self): with self.assertRaises(ValueError): # 'gt' supports only numeric values Condition('x', 'foo', 'gt') + with self.assertRaises(ValueError): + # 'in' supports only iterable values + Condition('x', 123, 'in') # # Nested attrs tests @@ -50,7 +48,10 @@ class ConditionTestCase(TestCase): c = Condition('x.y.z', 1) self.assertTrue(c.eval({'x': {'y': {'z': 1}}})) self.assertFalse(c.eval({'x': {'y': {'z': 2}}})) - self.assertFalse(c.eval({'a': {'b': {'c': 1}}})) + with self.assertRaises(InvalidCondition): + c.eval({'x': {'y': None}}) + with self.assertRaises(InvalidCondition): + c.eval({'x': {'y': {'a': 1}}}) # # Operator tests @@ -74,23 +75,31 @@ class ConditionTestCase(TestCase): c = Condition('x', 1, 'gt') self.assertTrue(c.eval({'x': 2})) self.assertFalse(c.eval({'x': 1})) + with self.assertRaises(InvalidCondition): + c.eval({'x': 'foo'}) # Invalid type def test_gte(self): c = Condition('x', 1, 'gte') self.assertTrue(c.eval({'x': 2})) self.assertTrue(c.eval({'x': 1})) self.assertFalse(c.eval({'x': 0})) + with self.assertRaises(InvalidCondition): + c.eval({'x': 'foo'}) # Invalid type def test_lt(self): c = Condition('x', 2, 'lt') self.assertTrue(c.eval({'x': 1})) self.assertFalse(c.eval({'x': 2})) + with self.assertRaises(InvalidCondition): + c.eval({'x': 'foo'}) # Invalid type def test_lte(self): c = Condition('x', 2, 'lte') self.assertTrue(c.eval({'x': 1})) self.assertTrue(c.eval({'x': 2})) self.assertFalse(c.eval({'x': 3})) + with self.assertRaises(InvalidCondition): + c.eval({'x': 'foo'}) # Invalid type def test_in(self): c = Condition('x', [1, 2, 3], 'in') @@ -106,6 +115,8 @@ class ConditionTestCase(TestCase): c = Condition('x', 1, 'contains') self.assertTrue(c.eval({'x': [1, 2, 3]})) self.assertFalse(c.eval({'x': [2, 3, 4]})) + with self.assertRaises(InvalidCondition): + c.eval({'x': 123}) # Invalid type def test_contains_negated(self): c = Condition('x', 1, 'contains', negate=True)