Merge pull request #234 from dotkernel/issue-231

refactor checking for a public or private ip

Author: arhimede

src/App/src/Service/IpService.php

@@ -7,8 +7,10 @@
 use function filter_var;
 use function getenv;
 
+use const FILTER_FLAG_IPV4;
 use const FILTER_FLAG_IPV6;
 use const FILTER_FLAG_NO_PRIV_RANGE;
+use const FILTER_FLAG_NO_RES_RANGE;
 use const FILTER_VALIDATE_IP;
 
 class IpService
@@ -17,20 +19,20 @@ public static function getUserIp(array $server): mixed
     {
         if (! empty($server)) {
             // check if HTTP_X_FORWARDED_FOR is public network IP
-            if (isset($server['HTTP_X_FORWARDED_FOR']) && self::validIp($server['HTTP_X_FORWARDED_FOR']) === 'public') {
+            if (isset($server['HTTP_X_FORWARDED_FOR']) && self::isPublicIp($server['HTTP_X_FORWARDED_FOR'])) {
                 $realIp = $server['HTTP_X_FORWARDED_FOR'];
             // check if HTTP_CLIENT_IP is public network IP
-            } elseif (isset($server['HTTP_CLIENT_IP']) && self::validIp($server['HTTP_CLIENT_IP']) === 'public') {
+            } elseif (isset($server['HTTP_CLIENT_IP']) && self::isPublicIp($server['HTTP_CLIENT_IP'])) {
                 $realIp = $server['HTTP_CLIENT_IP'];
             } else {
                 $realIp = $server['REMOTE_ADDR'];
             }
         } else {
             // check if HTTP_X_FORWARDED_FOR is public network IP
-            if (getenv('HTTP_X_FORWARDED_FOR') && self::validIp(getenv('HTTP_X_FORWARDED_FOR')) === 'public') {
+            if (getenv('HTTP_X_FORWARDED_FOR') && self::isPublicIp(getenv('HTTP_X_FORWARDED_FOR'))) {
                 $realIp = getenv('HTTP_X_FORWARDED_FOR');
             // check if HTTP_CLIENT_IP is public network IP
-            } elseif (getenv('HTTP_CLIENT_IP') && self::validIp(getenv('HTTP_CLIENT_IP')) === 'public') {
+            } elseif (getenv('HTTP_CLIENT_IP') && self::isPublicIp(getenv('HTTP_CLIENT_IP'))) {
                 $realIp = getenv('HTTP_CLIENT_IP');
             } else {
                 $realIp = getenv('REMOTE_ADDR');
@@ -40,27 +42,15 @@ public static function getUserIp(array $server): mixed
         return $realIp;
     }
 
-    /**
-     * @return false|string
-     */
-    public static function validIp(string $ip): bool|string
+    public static function isPublicIp(string $ipAddress): bool
     {
-        // special cases that return private are the loop-back address and IPv6 addresses
-        if ($ip === '127.0.0.1' || filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6)) {
-            return 'private';
-        }
-
-        // check if the ip is valid
-        if (filter_var($ip, FILTER_VALIDATE_IP)) {
-            // check whether it's private or not
-            if (filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE)) {
-                return 'public';
-            }
-
-            return 'private';
-        }
-
-        // not a valid ip
-        return false;
+        return filter_var(
+            $ipAddress,
+            FILTER_VALIDATE_IP,
+            FILTER_FLAG_IPV4 |
+                FILTER_FLAG_IPV6 |
+                FILTER_FLAG_NO_PRIV_RANGE |
+                FILTER_FLAG_NO_RES_RANGE
+        ) === $ipAddress;
     }
 }

test/Unit/Admin/Controller/AdminControllerTest.php

@@ -23,6 +23,13 @@ class AdminControllerTest extends UnitTest
      */
     public function testWillCreate(): void
     {
+        $logger          = new Logger([
+            'writers' => [
+                'FileWriter' => [
+                    'name' => 'null',
+                ],
+            ],
+        ]);
         $adminController = new AdminController(
             $this->createMock(AdminServiceInterface::class),
             $this->createMock(RouterInterface::class),
@@ -31,7 +38,7 @@ public function testWillCreate(): void
             $this->createMock(FlashMessengerInterface::class),
             $this->createMock(FormsPlugin::class),
             $this->createMock(AdminForm::class),
-            $this->createMock(Logger::class),
+            $logger
         );
         $this->assertInstanceOf(AdminController::class, $adminController);
     }

test/Unit/App/Service/IpServiceTest.php

@@ -49,12 +49,14 @@ public function testWillGetUserIpFromEnv(): void
         putenv('REMOTE_ADDR');
     }
 
-    public function testWillDetectIfValidIp(): void
+    public function testWillDetectPublicIp(): void
     {
-        $this->assertSame('private', IpService::validIp('127.0.0.1'));
+        $this->assertFalse(IpService::isPublicIp("127.0.0.1"));
+        $this->assertFalse(IpService::isPublicIp("10.0.0.0"));
+        $this->assertFalse(IpService::isPublicIp("::1"));
+        $this->assertFalse(IpService::isPublicIp("fd12:3456:789a:1::1"));
 
-        $this->assertSame('public', IpService::validIp($this->ipAddress));
-
-        $this->assertFalse(IpService::validIp('test'));
+        $this->assertTrue(IpService::isPublicIp("8.8.8.8")); // google
+        $this->assertTrue(IpService::isPublicIp("2607:f8b0:4003:c00::6a")); //google
     }
 }