-
-
Notifications
You must be signed in to change notification settings - Fork 7k
Security: Mitigate Recursion-based DoS in ListField and DictField #9858
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
acc3fd7
6ad1096
8ff5b53
fc5c1a6
60d4c60
af7d5f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1650,14 +1650,16 @@ class ListField(Field): | |
| 'not_a_list': _('Expected a list of items but got type "{input_type}".'), | ||
| 'empty': _('This list may not be empty.'), | ||
| 'min_length': _('Ensure this field has at least {min_length} elements.'), | ||
| 'max_length': _('Ensure this field has no more than {max_length} elements.') | ||
| 'max_length': _('Ensure this field has no more than {max_length} elements.'), | ||
| 'max_depth': _('List nesting depth exceeds maximum allowed depth of {max_depth}.') | ||
| } | ||
|
|
||
| def __init__(self, **kwargs): | ||
| self.child = kwargs.pop('child', copy.deepcopy(self.child)) | ||
| self.allow_empty = kwargs.pop('allow_empty', True) | ||
| self.max_length = kwargs.pop('max_length', None) | ||
| self.min_length = kwargs.pop('min_length', None) | ||
| self.max_depth = kwargs.pop('max_depth', None) | ||
|
|
||
| assert not inspect.isclass(self.child), '`child` has not been instantiated.' | ||
| assert self.child.source is None, ( | ||
|
|
@@ -1666,6 +1668,8 @@ def __init__(self, **kwargs): | |
| ) | ||
|
|
||
| super().__init__(**kwargs) | ||
| self._current_depth = 0 | ||
| self._root_max_depth = self.max_depth | ||
| self.child.bind(field_name='', parent=self) | ||
| if self.max_length is not None: | ||
| message = lazy_format(self.error_messages['max_length'], max_length=self.max_length) | ||
|
|
@@ -1674,6 +1678,29 @@ def __init__(self, **kwargs): | |
| message = lazy_format(self.error_messages['min_length'], min_length=self.min_length) | ||
| self.validators.append(MinLengthValidator(self.min_length, message=message)) | ||
|
|
||
| def bind(self, field_name, parent): | ||
| super().bind(field_name, parent) | ||
| if self.max_depth is None and hasattr(parent, '_root_max_depth') and parent._root_max_depth is not None: | ||
| self._root_max_depth = parent._root_max_depth | ||
mahdirahimi1999 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| self._current_depth = parent._current_depth + 1 | ||
| self._propagate_depth_to_child() | ||
|
|
||
| def _propagate_depth_to_child(self): | ||
| if self._root_max_depth is not None and hasattr(self.child, '_root_max_depth'): | ||
| self.child._root_max_depth = self._root_max_depth | ||
| self.child._current_depth = self._current_depth + 1 | ||
| if hasattr(self.child, '_propagate_depth_to_child'): | ||
| self.child._propagate_depth_to_child() | ||
auvipy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| def _check_data_depth(self, data, current_level): | ||
| items = data.values() if isinstance(data, dict) else data | ||
| for item in items: | ||
| if isinstance(item, (list, tuple, dict)): | ||
| next_level = current_level + 1 | ||
| if next_level > self._root_max_depth: | ||
| self.fail('max_depth', max_depth=self._root_max_depth) | ||
| self._check_data_depth(item, next_level) | ||
|
Comment on lines
+1695
to
+1702
|
||
|
|
||
| def get_value(self, dictionary): | ||
| if self.field_name not in dictionary: | ||
| if getattr(self.root, 'partial', False): | ||
|
|
@@ -1699,6 +1726,12 @@ def to_internal_value(self, data): | |
| self.fail('not_a_list', input_type=type(data).__name__) | ||
| if not self.allow_empty and len(data) == 0: | ||
| self.fail('empty') | ||
| if self._root_max_depth is not None: | ||
| start_level = self._current_depth if self._current_depth > 0 else 1 | ||
| if start_level > self._root_max_depth: | ||
| self.fail('max_depth', max_depth=self._root_max_depth) | ||
| if self.max_depth is not None: | ||
| self._check_data_depth(data, start_level) | ||
| return self.run_child_validation(data) | ||
|
|
||
| def to_representation(self, data): | ||
|
|
@@ -1730,11 +1763,13 @@ class DictField(Field): | |
| default_error_messages = { | ||
| 'not_a_dict': _('Expected a dictionary of items but got type "{input_type}".'), | ||
| 'empty': _('This dictionary may not be empty.'), | ||
| 'max_depth': _('Dictionary nesting depth exceeds maximum allowed depth of {max_depth}.') | ||
| } | ||
|
|
||
| def __init__(self, **kwargs): | ||
| self.child = kwargs.pop('child', copy.deepcopy(self.child)) | ||
| self.allow_empty = kwargs.pop('allow_empty', True) | ||
| self.max_depth = kwargs.pop('max_depth', None) | ||
|
|
||
| assert not inspect.isclass(self.child), '`child` has not been instantiated.' | ||
| assert self.child.source is None, ( | ||
|
|
@@ -1743,8 +1778,33 @@ def __init__(self, **kwargs): | |
| ) | ||
|
|
||
| super().__init__(**kwargs) | ||
| self._current_depth = 0 | ||
| self._root_max_depth = self.max_depth | ||
| self.child.bind(field_name='', parent=self) | ||
|
|
||
| def bind(self, field_name, parent): | ||
| super().bind(field_name, parent) | ||
| if self.max_depth is None and hasattr(parent, '_root_max_depth') and parent._root_max_depth is not None: | ||
| self._root_max_depth = parent._root_max_depth | ||
| self._current_depth = parent._current_depth + 1 | ||
| self._propagate_depth_to_child() | ||
|
|
||
| def _propagate_depth_to_child(self): | ||
| if self._root_max_depth is not None and hasattr(self.child, '_root_max_depth'): | ||
| self.child._root_max_depth = self._root_max_depth | ||
| self.child._current_depth = self._current_depth + 1 | ||
| if hasattr(self.child, '_propagate_depth_to_child'): | ||
| self.child._propagate_depth_to_child() | ||
|
|
||
| def _check_data_depth(self, data, current_level): | ||
| items = data.values() if isinstance(data, dict) else data | ||
| for item in items: | ||
| if isinstance(item, (list, tuple, dict)): | ||
| next_level = current_level + 1 | ||
| if next_level > self._root_max_depth: | ||
| self.fail('max_depth', max_depth=self._root_max_depth) | ||
| self._check_data_depth(item, next_level) | ||
|
|
||
| def get_value(self, dictionary): | ||
| # We override the default field access in order to support | ||
| # dictionaries in HTML forms. | ||
|
|
@@ -1762,7 +1822,12 @@ def to_internal_value(self, data): | |
| self.fail('not_a_dict', input_type=type(data).__name__) | ||
| if not self.allow_empty and len(data) == 0: | ||
| self.fail('empty') | ||
|
|
||
| if self._root_max_depth is not None: | ||
| start_level = self._current_depth if self._current_depth > 0 else 1 | ||
| if start_level > self._root_max_depth: | ||
| self.fail('max_depth', max_depth=self._root_max_depth) | ||
| if self.max_depth is not None: | ||
| self._check_data_depth(data, start_level) | ||
| return self.run_child_validation(data) | ||
|
|
||
| def to_representation(self, value): | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think adding a default limit for max_depth would be a good idea? Users could still override it if needed, but having a cushion would help even if they don't set a limit. What’s your take on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered that, but a default limit might break existing setups that legitimately use deep nesting. Keeping it None by default feels safer for backward compatibility. That said, we should definitely highlight it in the docs as a security best practice.