Refactor of IP address validation logic#44
Conversation
This commit introduces new test cases in the `ValidateTests.cs` file for the following methods: - `ValidIpv4ORCIDR` - `ValidIpv4ORIpv6ORCIDR` - `ValidIpv4CIDR` Each test case includes the data row `0.0.0.0/1` to ensure proper validation of this CIDR notation.
Updates test cases in to cover new scenarios for both invalid and valid IPv4 and CIDR inputs. Removed some duplicate DataRow test cases.
Renamed validation methods in the `Validate` class to differentiate valid endpoint addresses from other valid address formats. Updated usages of the validation code where appropriate to differentiate between when the code path wanted to check for a valid IP Address in the context of an endpoint vs a route (where things like 0.0.0.0 would be allowed) Refactored main IP address validation logic to use a single internal function to ensure consistency of validation across address types. Test cases in `ValidateTests.cs` were modified to reflect the new method names and ensure proper functionality.
|
@ld0614 Ok here's a more comprehensive coverage. I tried running the full unit test set, but I dont think my machine is properly set up for it. I'm getting validation errors about |
|
Also, this supersedes the other PR, however is a lot more code change. It's an option for you to consider. If you prefer the original approach, I'm happy to make the changes best I can there. |
|
Thanks for submitting this, I've done a quick eyeball over it and it looks like a much cleaner approach tbh. Hopefully I'll have some time over the weekend to really sit down and see the code in context + do a full test suite run. On that subject, are you using the Run-TestsAsSYSTEM.ps1 script? This has to run on a machine with full admin rights and no other VPNs configured as a large chunk of the testing it is provisioning and deleting various VPN configurations as Windows has some 'unique' behaviours that are 'not well documented' so the only real way to validate things fully is to send them to windows and see if it breaks... |
|
That makes sense. I might need to set up a dedicated VM for testing. My dev box is server 2025 and am a bit scared to run those tests locally 😅 |
|
Hey @ryannewington Tests have fully passed 😄 I do all my DPC dev in a Windows 11 VM as I don't want it trashing my laptop with the number of VPN profiles that it creates and deletes, not to mention the amount of registry modifications the tests make to trigger the different tests... A couple of things to note from my full code review:
|
Add validation for rejecting non v4 or v6 address families Moves CIDR validation routines to use new internal address validation method
|
I've removed the ternary expression and rejected non v4/v6 address families. I originally didnt do this because its an internal method and we're in control of the callers, but it doesn't hurt to add it. I might be going mad, but I can't find any extra whitespace - even with that plugin - can you point me to where you are seeing it? I also noticed that I forgot to move the IPv4CIDR and IPc6CIDR validation methods to use the new internal function! I've updated those as well. Apologies! |
|
Good catch on the CIDR functions, I completely overlooked them 😄 My general philosophy with DPC has been to treat the developer as an idiot (because I often can be 😉) and performance is mostly irrelevant (within reason) as the cycles to perform some extra (or repeated) checks are nothing compared to the cycles Windows needs to mess with a VPN profile. As its all silent and in the background, as long as the profile update behaviour doesn't spike the CPU and doesn't take more than a few seconds no one is going to notice. It looks like your commit removed the white space I had noticed I'll run another full test suite run on the code overnight (now) and then if your happy with it, I'll merge at some point tomorrow Thanks for all your time on this, it really is appreciated and really nice to be working with someone else on this project for once |
* Update Copyright date in EULA * Add simple interactive test endpoint for debugging. This should never ship in the released build * Added detection and removal for corrupted hiddenPBK files * Increment build number as the beta version of 5.0.4 got rolled out to a number of users * Avoid setting device tunnel related registry setting unless DPC 'owns' the device tunnel config. fixes #27 * Add support for Excluding DNS entries from force tunnel #26 and initial test * Avoid issues where multiple DNS entries return the same IP addresses * Update Readme and bump dependencies * Update ETW Library to fix issues with operational logs disappearing * Added ability to write logs to disk * Fix Operational Events not showing in event log by forcing a reorganisation of the auto-generated Manifest file into the correct channel order * Identified and fixed support for WMI profiles correctly identifying when Disable Edit UI and Disable UI Disconnect options #3 * Update README in preperation for release * Update README with Feature already added to the development branch * Refactor DNS lookups to continue in the event of a single resolution issue. Add more tests for DNS Exclusion scenarios * Bump Version Number * Add tests for duplicate IP support and fix bug with DNS route comments * Update README * Clean up Dev Client * Fix DNS route null exceptions * bump version number * Attempt to avoid errors during the GPO update process by staggering profile updates #31 * Add additional Null Check * Fix missing null check * Add initial Get-RRASReport provided by ChrisAtWork on Discord * Fix bug where thread was locked for profile update time * Add debug event to show why a profile is being updated and prepare for 5.2.0 release * Update comments and fix capitalization * Fixed issue with profile update never returning spinlock * Updated proxy examples * Fix incorrect registry location for User Backup Tunnel Setting * Fix null pointer exception * Update interim changelog * Update PSExec Scripts to dynamically locate the latest version of the MS Store Sysinternals packages * Tests for and fixed #42 * Update Route to make use of existing IPUtil methods and add in additional safety checks * update comment * Refactor of IP address validation logic (#44) * Improve CIDR validation logic for IPv4 addresses * Add test cases for 0.0.0.0/1 CIDR notation This commit introduces new test cases in the `ValidateTests.cs` file for the following methods: - `ValidIpv4ORCIDR` - `ValidIpv4ORIpv6ORCIDR` - `ValidIpv4CIDR` Each test case includes the data row `0.0.0.0/1` to ensure proper validation of this CIDR notation. * Update development pipeline to run for all branches other than main * Adds support for validating CIDR 0.0.0.0/0 Updates test cases in to cover new scenarios for both invalid and valid IPv4 and CIDR inputs. Removed some duplicate DataRow test cases. * Refactor IP address validation methods Renamed validation methods in the `Validate` class to differentiate valid endpoint addresses from other valid address formats. Updated usages of the validation code where appropriate to differentiate between when the code path wanted to check for a valid IP Address in the context of an endpoint vs a route (where things like 0.0.0.0 would be allowed) Refactored main IP address validation logic to use a single internal function to ensure consistency of validation across address types. Test cases in `ValidateTests.cs` were modified to reflect the new method names and ensure proper functionality. * Rename validation tests for IPv4 and IPv6 endpoints * Update ValidateIPInternal logic to not use ternary expression Add validation for rejecting non v4 or v6 address families Moves CIDR validation routines to use new internal address validation method --------- Co-authored-by: Leo D'Arcy <leo.darcy@outlook.com> * Update interim change log * Add additional null checks for proxy exclusions * Prepare for release --------- Co-authored-by: Leo D'Arcy <leo.darcy@PowerONPlatforms.com> Co-authored-by: Ryan Newington <ryan@lithiumblue.com>
2 participants
IPAddress.TryParseIPvXEndpointAddressvsIPvXAddress. The "endpoint address" variants expect an addressable IP address, whereas the other accepts well-formed but non-addressable IP addresses (eg used in routes such as 0.0.0.0 and ::)Validate.IPvXto use the correct new function, depending on context of the callerBasicUserProfileWithSillyRoutestoBasicUserProfileWithDefaultRoutes- I'm not sure what other cases to substitute with the 'silly' route