-
Notifications
You must be signed in to change notification settings - Fork 3
Fix for split_rules_for_user #66
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
Conversation
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.
Pull Request Overview
This PR adds robust error handling for attribute access in rule filtering functions to handle cases where rule objects may be missing expected attributes like source, dest, or action_id.
- Wrapped attribute access in try-catch blocks for
filter_rules_in_network,split_rules_for_user, andfilter_rules_actionfunctions - Added comprehensive test cases for edge cases with missing or incomplete rule attributes
- Updated version to 1.1.4 with changelog entry
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| flowapp/validators.py | Added try-catch blocks around attribute access for filter_rules_in_network and split_rules_for_user |
| flowapp/flowspec.py | Added try-catch block around rule.action_id access in filter_rules_action |
| flowapp/tests/test_validators.py | Added extensive test cases for robust attribute handling with mock rule classes |
| flowapp/about.py | Version bump from 1.1.3 to 1.1.4 |
| README.md | Added changelog entry for version 1.1.4 |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| try: | ||
| if network_in_range(rule.source, rule.source_mask, net_ranges) or network_in_range( | ||
| rule.dest, rule.dest_mask, net_ranges | ||
| ): | ||
| user_rules.append(rule) | ||
| else: | ||
| rest_rules.append(rule) | ||
| except AttributeError: | ||
| # If rule has no source or dest, the split is not possible | ||
| user_rules.append(rule) |
Copilot
AI
Sep 25, 2025
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.
The try-catch block only catches AttributeError but not other exceptions that network_in_range might raise. Based on the context, network_in_range can raise ValueError for invalid network formats, which would not be caught here and could cause unexpected crashes.
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.
ValueError is catched in the network_in_range validator
def network_in_range(address, mask, net_ranges):
"""
check if given ip address is in user network ranges
:param address: string ip_address
:param net_ranges: list of network ranges
:return: boolean
"""
result = False
network = "{}/{}".format(address, mask)
for adr_range in net_ranges:
try:
result = result or subnet_of(ipaddress.ip_network(network), ipaddress.ip_network(adr_range))
except TypeError: # V4 can't be a subnet of V6 and vice versa
pass
except ValueError:
return False
return result
Co-authored-by: Copilot <[email protected]>
More robust version for spiting rules for non-admin users.