Skip to content

Conversation

@jirivrany
Copy link
Collaborator

More robust version for spiting rules for non-admin users.

@jirivrany jirivrany requested a review from Copilot September 25, 2025 10:47
Copy link
Contributor

Copilot AI left a 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, and filter_rules_action functions
  • 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.

Comment on lines +36 to 45
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)
Copy link

Copilot AI Sep 25, 2025

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.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

@jirivrany jirivrany Sep 25, 2025

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

@jirivrany jirivrany merged commit 0cf0bc1 into main Sep 25, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants