Fix IPv6 non-specified ranges unexpectedly allowed

[Jimadine]

As reported in #1821.

As alluded to in the issue text, I am entirely happy for someone to pull my changes apart and rewrite. We are already using the modified QubitLimitIp.class.php in production, and it meets our basic needs.

For the future: it would be a good idea to also validate the input box content so that the administrator adding IPs/ranges can be sure what they have entered meets the required format. I've created a simple example of how this could be done in Javascript, but it could be implemented differently e.g. entirely server-side.

[anvit]

Hi @Jimadine ! Thank you so much for both, reporting this issue and submitting this PR to fix it. We'll review the PR soon, and if everything looks good, get this fix in for the next release.

When you get a chance, would you be able to sign our Contributor Agreement? Here's a link with details about it agreement —
https://wiki.accesstomemory.org/Development/Contribute_code#Copyright_and_license

[Jimadine]

Hi @Jimadine ! Thank you so much for both, reporting this issue and submitting this PR to fix it. We'll review the PR soon, and if everything looks good, get this fix in for the next release.

You're very welcome! 😄

When you get a chance, would you be able to sign our Contributor Agreement? Here's a link with details about it agreement — https://wiki.accesstomemory.org/Development/Contribute_code#Copyright_and_license

I've just this moment emailed my signed agreement to [email protected]

[anvit]

@Jimadine yup, we received it. Thanks!

[anvit]

@Jimadine I just looked at your changes and this fix makes sense to me. I noticed that our syntax checks are failing (probably indentation related), and we like to keep our commit message header to 50 characters but the rest looks good! Would you be able to fix those? If not, I'm happy to add a commit fixing those and then accept the changes!

[Jimadine]

@Jimadine I just looked at your changes and this fix makes sense to me. I noticed that our syntax checks are failing (probably indentation related), and we like to keep our commit message header to 50 characters but the rest looks good! Would you be able to fix those? If not, I'm happy to add a commit fixing those and then accept the changes!

Hi @anvit - I have amended the commit message and fixed the indentation. Hopefully the syntax checks will pass now 😃.

[anvit]

@Jimadine unfortunately syntax checks are still failing. I found the lines it was complaining about and flagged them in the review.

[anvit]

Syntax checks seem to be failing due to missing new line before return statement. Please add an empty new line before the return here.

[Jimadine]

After adding the extra blank line, CS fixer complained about it:

root@ubuntu-focal:/usr/share/nginx/atom# php composer.phar php-cs-fixer fix /usr/share/nginx/atom/lib/filter/QubitLimitIp.class.php -v
Do not run Composer as root/super user! See https://getcomposer.org/root for details
Continue as root/super user [yes]?
> php-cs-fixer: php-cs-fixer 'fix' '/usr/share/nginx/atom/lib/filter/QubitLimitIp.class.php'
PHP CS Fixer 3.59.3 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 7.4.3-4ubuntu2.23
Running analysis on 1 core sequentially.
You can enable parallel runner and speed up the analysis! Please see usage docs for more information.
Loaded config default from "/usr/share/nginx/atom/.php-cs-fixer.dist.php".
Using cache file ".php-cs-fixer.cache".
Paths from configuration file have been overridden by paths provided as command arguments.
 1/1 [...] 100%

   1) lib/filter/QubitLimitIp.class.php (no_extra_blank_lines)

Fixed 1 of 1 files in 0.125 seconds, 18.00 MB memory used
root@ubuntu-focal:/usr/share/nginx/atom# git diff lib/filter/QubitLimitIp.class.php
diff --git a/lib/filter/QubitLimitIp.class.php b/lib/filter/QubitLimitIp.class.php
index 592277d..cb0dbf6 100644
--- a/lib/filter/QubitLimitIp.class.php
+++ b/lib/filter/QubitLimitIp.class.php
@@ -90,7 +90,6 @@ class QubitLimitIpFilter extends sfFilter
                     (strlen($addressBinary) == strlen($firstInRangeBinary))
                      && ($addressBinary >= $firstInRangeBinary && $addressBinary <= $lastInRangeBinary)
                 ) {
-
                     return true;
                 }
             }

...so I went with the CS fixer fix to remove it, and now the syntax check passes 🚀

[anvit]

Syntax checker also asks to remove this empty line.

[Jimadine]

@anvit - thank you for identifying what needs to be changed 😄. When I ran PHP CS fixer with the default rules, it made the following change:

diff --git a/lib/filter/QubitLimitIp.class.php b/lib/filter/QubitLimitIp.class.php
index 41b32ed06..ff053f4db 100644
--- a/lib/filter/QubitLimitIp.class.php
+++ b/lib/filter/QubitLimitIp.class.php
@@ -89,7 +89,7 @@ class QubitLimitIpFilter extends sfFilter
                 if (
                     (strlen($addressBinary) == strlen($firstInRangeBinary))
                      && ($addressBinary >= $firstInRangeBinary && $addressBinary <= $lastInRangeBinary)
-                   ) {
+                ) {
                     return true;
                 }
             }

But it didn't make the changes you've identified. I'm wondering if that's because there are customisations to the rules. I can see a rule in .php-cs-fixer.dist.php, in the root of the repo, but even when I reference this file with --cache-file, I get the same result. This is the output I get:

25/06/2024  9:15:54> - C:\Users\jea9\AppData\Local\Programs\php-8.3.8-nts-Win32-vs16-x64>php php-cs-fixer.phar fix C:\Jims_file_repository\Version_control\GitHub\atom-york\lib\filter\QubitLimitIp.class.php --cache-file C:\Jims_file_repository\Version_control\GitHub\atom-york\.php-cs-fixer.dist.php
PHP CS Fixer 3.59.3 (064efa1) 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 8.3.8
Running analysis on 1 core sequentially.
You can enable parallel runner and speed up the analysis! Please see usage docs for more information.
Loaded config default.
Using cache file "C:\Jims_file_repository\Version_control\GitHub\atom-york\.php-cs-fixer.dist.php".
 1/1 [...] 100%


Fixed 0 of 1 files in 0.004 seconds, 20.00 MB memory used

Do you have any thoughts on why PHP CS fixer isn't identifying/fixing those lines for me? No worries if not; I can make the changes manually.

[anvit]

@Jimadine Hmm… I'm not entirely sure but my guess is that the rules aren't being picked up? I usually run it via the composer script like this composer php-cs-fix -- fix -v. There's another thing I noticed. It says you're running php 8.3.8. Do you usually do all the development work for AtoM directly on your machine? Or do you work through a docker container? Because AtoM uses an older versions of PHP (7.4) and composer (3.6.0), so I wonder if either of those are factors as well. If you're using docker, you can run it using docker compose exec atom composer php-cs-fix -- fix -v

[Jimadine]

@anvit - I don't usually do AtoM development, so I downloaded the latest version of PHP and php-cs-fixer.phar to a folder on my Windows machine. My testing has been on our Ubuntu 20.04 VMWare test server. I tend to use Vagrant a lot, though more for deployment testing purposes.

For this purpose, I've spun up a Vagrant Ubuntu 20.04 box with PHP 7.4, installed the composer dependencies with php composer.phar update, and tried again to see what PHP CS fixer will fix. This time it made the following corrections (on top of the previous undentation):

root@ubuntu-focal:/usr/share/nginx/atom# php composer.phar php-cs-fixer fix /usr/share/nginx/atom/lib/filter/QubitLimitIp.class.php
Do not run Composer as root/super user! See https://getcomposer.org/root for details
Continue as root/super user [yes]? yes
> php-cs-fixer 'fix' '/usr/share/nginx/atom/lib/filter/QubitLimitIp.class.php'
PHP CS Fixer 3.59.3 7th Gear by Fabien Potencier, Dariusz Ruminski and contributors.
PHP runtime: 7.4.3-4ubuntu2.23
Running analysis on 1 core sequentially.
You can enable parallel runner and speed up the analysis! Please see usage docs for more information.
Loaded config default from "/usr/share/nginx/atom/.php-cs-fixer.dist.php".
Using cache file ".php-cs-fixer.cache".
Paths from configuration file have been overridden by paths provided as command arguments.
 1/1 [...] 100%

   1) lib/filter/QubitLimitIp.class.php

Fixed 1 of 1 files in 0.142 seconds, 18.00 MB memory used
root@ubuntu-focal:/usr/share/nginx/atom# git diff lib/filter/QubitLimitIp.class.php
diff --git a/lib/filter/QubitLimitIp.class.php b/lib/filter/QubitLimitIp.class.php
index ff053f4..cb0dbf6 100644
--- a/lib/filter/QubitLimitIp.class.php
+++ b/lib/filter/QubitLimitIp.class.php
@@ -94,7 +94,7 @@ class QubitLimitIpFilter extends sfFilter
                 }
             }
         }
+
         return false;
     }
-
 }

So in addition to removing line 99 that you identified, it wants a new line at line 97, before the return false;. I've made these changes, and I've also added the extra new line at line 93, as you mentioned.

I'm not sure why I'm seeing these discrepancies. I have installed the latest Composer version 2.7.7 2024-06-10 22:11:12. When I run php composer.phar php-cs-fixer -- fix it fixes lots of files.

I see the syntax checking has failed on my fork again 😞.

[Jimadine]

@anvit - I think all should be well now - see my comment.

One for a new issue perhaps, but it might be helpful to add the -v option to the PHP CS fixer command in the Github workflow. Doing this appears to give a description of what the problem is, in the output, e.g.:

   1) lib/filter/QubitLimitIp.class.php (no_extra_blank_lines)

It's not ideal that there's no line number reference, but at least it's something.

[anvit]

@Jimadine Thank you! Your last commit fixed the syntax check issues!