diff --git a/netbox/netbox/graphql/filter_lookups.py b/netbox/netbox/graphql/filter_lookups.py index d3da1239e..3419931bf 100644 --- a/netbox/netbox/graphql/filter_lookups.py +++ b/netbox/netbox/graphql/filter_lookups.py @@ -1,3 +1,4 @@ +import re from enum import Enum from typing import Generic, TypeVar @@ -13,12 +14,46 @@ from strawberry_django import ( ComparisonFilterLookup, FilterLookup, RangeLookup, - StrFilterLookup, process_filters, ) from netbox.graphql.scalars import BigInt +# ------------------------------------------------------------------ +# JSON path validation (VM-323) +# ------------------------------------------------------------------ + +# Each segment of a JSON path may only contain alphanumerics, underscores, and +# hyphens. Hyphens are included because JSON keys commonly use them; leading +# underscores are permitted (e.g. _foo is a valid key name). +_JSON_PATH_SEGMENT_RE = re.compile(r'^[A-Za-z0-9_][A-Za-z0-9_-]*$') + + +def _validate_json_path(path: str) -> str: + """Validate a JSON traversal path for use in ORM lookups. + + Each ``__``-separated segment must match ``[A-Za-z0-9_][A-Za-z0-9_-]*``. + Raises ``ValueError`` on an empty path, empty segment, or segment with + disallowed characters. + + ORM operator names (``date``, ``regex``, etc.) are intentionally *not* + blocked here: ``JSONFilter.filter()`` always appends ``__`` to the path + before handing it to ``process_filters``, so a segment named ``regex`` + becomes another level of JSON key traversal (``data__key__regex__exact``), + not the ORM regex transform (``data__key__regex=…``). + """ + if not path: + raise ValueError("JSON path cannot be empty") + + for segment in path.split('__'): + if not segment: + raise ValueError("JSON path contains consecutive or trailing '__'") + if not _JSON_PATH_SEGMENT_RE.match(segment): + raise ValueError(f"Invalid JSON path segment: {segment!r}") + + return path + + __all__ = ( 'ArrayLookup', 'BigIntegerLookup', @@ -28,6 +63,8 @@ __all__ = ( 'IntegerLookup', 'IntegerRangeArrayLookup', 'JSONFilter', + 'JSONLookup', + 'JSONStringLookup', 'StringArrayLookup', 'TreeNodeFilter', ) @@ -78,9 +115,33 @@ class JSONDatetimeFilterLookup(ComparisonFilterLookup[str]): time: ComparisonFilterLookup[int] | None = strawberry.UNSET +@strawberry.input(description='String lookups for JSON field values.') +class JSONStringLookup: + """ + String-filter type for use inside JSONLookup. + + Equivalent to ``StrFilterLookup`` but defined explicitly so that the type + name remains stable and any future per-field restrictions are easy to add. + ``regex`` / ``i_regex`` are included: they provide no additional oracle + power beyond ``starts_with``, which is also present. + """ + exact: str | None = strawberry_django.filter_field() + i_exact: str | None = strawberry_django.filter_field() + contains: str | None = strawberry_django.filter_field() + i_contains: str | None = strawberry_django.filter_field() + starts_with: str | None = strawberry_django.filter_field() + i_starts_with: str | None = strawberry_django.filter_field() + ends_with: str | None = strawberry_django.filter_field() + i_ends_with: str | None = strawberry_django.filter_field() + in_: list[str] | None = strawberry_django.filter_field() + isnull: bool | None = strawberry_django.filter_field() + regex: str | None = strawberry_django.filter_field() + i_regex: str | None = strawberry_django.filter_field() + + @strawberry.input(one_of=True, description='Lookup for JSON field. Only one of the lookup fields can be set.') class JSONLookup: - string_lookup: StrFilterLookup | None = strawberry_django.filter_field() + string_lookup: JSONStringLookup | None = strawberry_django.filter_field() int_range_lookup: RangeLookup[int] | None = strawberry_django.filter_field() int_comparison_lookup: ComparisonFilterLookup[int] | None = strawberry_django.filter_field() float_range_lookup: RangeLookup[float] | None = strawberry_django.filter_field() @@ -158,7 +219,12 @@ class JSONFilter: if not filters: return queryset, Q() - json_path = f'{prefix}{self.path}__' + try: + safe_path = _validate_json_path(self.path) + except ValueError: + return queryset, Q() + + json_path = f'{prefix}{safe_path}__' return process_filters(filters=filters, queryset=queryset, info=info, prefix=json_path) diff --git a/netbox/netbox/tests/test_graphql.py b/netbox/netbox/tests/test_graphql.py index 0b711348b..e65aca00f 100644 --- a/netbox/netbox/tests/test_graphql.py +++ b/netbox/netbox/tests/test_graphql.py @@ -505,3 +505,97 @@ class GraphQLAPITestCase(APITestCase): data = json.loads(response.content) self.assertIn('errors', data) self.assertEqual(data['errors'][0]['message'], 'Cannot specify both `start` and `offset` in pagination.') + + +class JSONPathValidationTestCase(TestCase): + """Unit tests for _validate_json_path (VM-323 security fix).""" + + def setUp(self): + from netbox.graphql.filter_lookups import _validate_json_path + self.validate = _validate_json_path + + # --- Valid paths --- + + def test_single_key(self): + self.assertEqual(self.validate('key'), 'key') + + def test_nested_key(self): + self.assertEqual(self.validate('parent__child'), 'parent__child') + + def test_deeply_nested(self): + self.assertEqual(self.validate('a__b__c'), 'a__b__c') + + def test_key_with_underscores(self): + self.assertEqual(self.validate('my_key'), 'my_key') + + def test_key_with_hyphens(self): + self.assertEqual(self.validate('my-key'), 'my-key') + + def test_numeric_array_index(self): + self.assertEqual(self.validate('items__0'), 'items__0') + + def test_alphanumeric_segment(self): + self.assertEqual(self.validate('key123'), 'key123') + + def test_key_with_leading_underscore(self): + # JSON keys may start with underscore (e.g. _foo) + self.assertEqual(self.validate('_key'), '_key') + + def test_orm_operator_name_as_key(self): + # 'date', 'regex' etc. are valid JSON key names; the path validator + # must not block them. The ORM injection risk is neutralised by the + # trailing __ that JSONFilter always appends before process_filters. + self.assertEqual(self.validate('date'), 'date') + self.assertEqual(self.validate('key__regex'), 'key__regex') + self.assertEqual(self.validate('key__exact'), 'key__exact') + + # --- Invalid paths --- + + def test_rejects_empty_string(self): + with self.assertRaises(ValueError): + self.validate('') + + def test_rejects_all_underscores(self): + # '___' splits into segments ['', '', ''] via '__' — empty segments rejected + with self.assertRaises(ValueError): + self.validate('___') + + def test_accepts_trailing_single_underscore(self): + # A single trailing underscore is a valid JSON key character + self.assertEqual(self.validate('key_'), 'key_') + + def test_rejects_trailing_double_underscore(self): + with self.assertRaises(ValueError): + self.validate('key__') + + def test_rejects_leading_double_underscore(self): + with self.assertRaises(ValueError): + self.validate('__key') + + def test_rejects_consecutive_double_underscores(self): + with self.assertRaises(ValueError): + self.validate('key1____key2') + + def test_rejects_segment_starting_with_special_char(self): + with self.assertRaises(ValueError): + self.validate('$secret') + + def test_rejects_path_with_spaces(self): + with self.assertRaises(ValueError): + self.validate('key one') + + def test_rejects_path_with_dot(self): + with self.assertRaises(ValueError): + self.validate('key.subkey') + + +class JSONStringLookupTestCase(TestCase): + """Verify JSONStringLookup exposes the expected set of string operators.""" + + def test_string_operators_present(self): + from netbox.graphql.filter_lookups import JSONStringLookup + field_names = {f.name for f in JSONStringLookup.__strawberry_definition__.fields} + for expected in ('exact', 'i_exact', 'contains', 'i_contains', + 'starts_with', 'i_starts_with', 'ends_with', 'i_ends_with', + 'in_', 'isnull', 'regex', 'i_regex'): + self.assertIn(expected, field_names, f"{expected!r} must be present on JSONStringLookup")