Skip to content
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

Improvements when detecting WebGUI access rules #294

Merged
merged 1 commit into from
Aug 17, 2024

Conversation

Martinski4GitHub
Copy link
Collaborator

Made code improvements to parse correctly the following use cases of WebGUI access rules:

  1. Using CIDR notation for a single LAN IP address (e.g. 192.168.100.1/32)
  2. Using CIDR notation for a block of LAN IP addresses (e.g. 192.168.100.0/28) using the network address.

This should take care of 99% of the valid use cases.

Made code improvements to parse correctly the following use cases of WebGUI access rules:

1) Using CIDR notation for a single IP address (e.g. 192.168.100.1/32)
2) Using CIDR notation for a block of IP addresses (e.g. 192.168.100.0/28)

This should take care of 99% of the valid use cases.
@Martinski4GitHub
Copy link
Collaborator Author

@ExtremeFiretop,
Please review & test whenever you have the time. So far, it's working well with my use cases.

@ExtremeFiretop
Copy link
Owner

ExtremeFiretop commented Aug 17, 2024

  1. Using CIDR notation for a block of LAN IP addresses (e.g. 192.168.100.0/28) using the network address.

This should take care of 99% of the valid use cases.

This is very good, a great catch, I honestly didn't even notice you could enter a network address range:

image

Only after reviewing this PR did I go... Why would we need to check a range? Logged in and sure enough saw you could approve a range, which means my method wouldn't catch a router that has access to itself just due to falling within that range that's been allowed.

Because the feature is limited to only 4 "entries"; I didn't expect a range to be allowed, I expected a 4 IP hard limit.

Approved and merged!

@ExtremeFiretop ExtremeFiretop merged commit 668f2a1 into ExtremeFiretop:dev Aug 17, 2024
1 check passed
@Martinski4GitHub
Copy link
Collaborator Author

  1. Using CIDR notation for a block of LAN IP addresses (e.g. 192.168.100.0/28) using the network address.

This should take care of 99% of the valid use cases.

This is very good, a great catch, I honestly didn't even notice you could enter a network address range:

image

Only after reviewing this PR did I go... Why would we need to check a range? Logged in and sure enough saw you could approve a range, which means my method wouldn't catch a router that has access to itself just due to falling within that range that's been allowed.

Because the feature is limited to only 4 "entries"; I didn't expect a range to be allowed, I expected a 4 IP hard limit.

Approved and merged!

As the saying goes: "The devil is in the details." Late last night, while testing the latest code for RMerlin F/W updates, I realized there are 2 more use cases that should be considered. Granted, they're not very common, but I know of one router installation where the network IP address is not the usual default. See the new PR #295 that I just submitted.

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