unify IP-related logic into traffic limiter

lib/Controller.php

@@ -195,35 +195,14 @@ class Controller
      */
     private function _create()
     {
-        // Check if whitelist feature is enabled
-        if (($option = $this->_conf->getKey('creators', 'traffic')) !== '') {
-            // Parse whitelist into array
-            $whitelist = explode(',', $option);
-            // Check for source IP in HTTP header
-            if (($option = $this->_conf->getKey('header', 'traffic')) !== null) {
-                $httpHeader = 'HTTP_' . $option;
-                // Grab source IP from HTTP header (if it exists)
-                if (array_key_exists($httpHeader, $_SERVER) && !empty($_SERVER[$httpHeader])) {
-                    // Check if source IP reported from HTTP header is in whitelist array
-                    if (!in_array($_SERVER[$httpHeader], $whitelist)) {
-                        $this->_return_message(1, I18n::_('Your IP is not authorized to create pastes.'));
-                        return;
-                    }
-                }
-            }
-        }
-
         // Ensure last paste from visitors IP address was more than configured amount of seconds ago.
         ServerSalt::setStore($this->_model->getStore());
         TrafficLimiter::setConfiguration($this->_conf);
         TrafficLimiter::setStore($this->_model->getStore());
-        if (!TrafficLimiter::canPass()) {
-            $this->_return_message(
-                1, I18n::_(
-                    'Please wait %d seconds between each post.',
-                    $this->_conf->getKey('limit', 'traffic')
-                )
-            );
+        try {
+            TrafficLimiter::canPass();
+        } catch (Exception $e) {
+            $this->_return_message(1, $e->getMessage());
             return;
         }
 

lib/Persistence/TrafficLimiter.php

@@ -13,8 +13,10 @@
 
 namespace PrivateBin\Persistence;
 
+use Exception;
 use IPLib\Factory;
 use PrivateBin\Configuration;
+use PrivateBin\I18n;
 
 /**
  * TrafficLimiter
@@ -24,13 +26,13 @@ use PrivateBin\Configuration;
 class TrafficLimiter extends AbstractPersistence
 {
     /**
-     * time limit in seconds, defaults to 10s
+     * listed IPs are the only ones allowed to create, defaults to null
      *
      * @access private
      * @static
-     * @var    int
+     * @var    string|null
      */
-    private static $_limit = 10;
+    private static $_creators = null;
 
     /**
      * listed IPs are exempted from limits, defaults to null
@@ -51,19 +53,49 @@ class TrafficLimiter extends AbstractPersistence
     private static $_ipKey = 'REMOTE_ADDR';
 
     /**
-     * set the time limit in seconds
+     * time limit in seconds, defaults to 10s
+     *
+     * @access private
+     * @static
+     * @var    int
+     */
+    private static $_limit = 10;
+
+    /**
+     * set configuration options of the traffic limiter
      *
      * @access public
      * @static
-     * @param  int $limit
+     * @param Configuration $conf
      */
-    public static function setLimit($limit)
+    public static function setConfiguration(Configuration $conf)
     {
-        self::$_limit = $limit;
+        self::setCreators($conf->getKey('creators', 'traffic'));
+        self::setExempted($conf->getKey('exempted', 'traffic'));
+        self::setLimit($conf->getKey('limit', 'traffic'));
+
+        if (($option = $conf->getKey('header', 'traffic')) !== '') {
+            $httpHeader = 'HTTP_' . $option;
+            if (array_key_exists($httpHeader, $_SERVER) && !empty($_SERVER[$httpHeader])) {
+                self::$_ipKey = $httpHeader;
+            }
+        }
     }
 
     /**
-     * set a list of IP(-ranges) as string
+     * set a list of creator IP(-ranges) as string
+     *
+     * @access public
+     * @static
+     * @param string $creators
+     */
+    public static function setCreators($creators)
+    {
+        self::$_creators = $creators;
+    }
+
+    /**
+     * set a list of exempted IP(-ranges) as string
      *
      * @access public
      * @static
@@ -75,23 +107,15 @@ class TrafficLimiter extends AbstractPersistence
     }
 
     /**
-     * set configuration options of the traffic limiter
+     * set the time limit in seconds
      *
      * @access public
      * @static
-     * @param Configuration $conf
+     * @param  int $limit
      */
-    public static function setConfiguration(Configuration $conf)
+    public static function setLimit($limit)
     {
-        self::setLimit($conf->getKey('limit', 'traffic'));
-        self::setExempted($conf->getKey('exempted', 'traffic'));
-
-        if (($option = $conf->getKey('header', 'traffic')) !== '') {
-            $httpHeader = 'HTTP_' . $option;
-            if (array_key_exists($httpHeader, $_SERVER) && !empty($_SERVER[$httpHeader])) {
-                self::$_ipKey = $httpHeader;
-            }
-        }
+        self::$_limit = $limit;
     }
 
     /**
@@ -108,7 +132,7 @@ class TrafficLimiter extends AbstractPersistence
     }
 
     /**
-     * Validate $_ipKey against configured ipranges. If matched we will ignore the ip
+     * validate $_ipKey against configured ipranges. If matched we will ignore the ip
      *
      * @access private
      * @static
@@ -136,22 +160,33 @@ class TrafficLimiter extends AbstractPersistence
     }
 
     /**
-     * traffic limiter
-     *
-     * Make sure the IP address makes at most 1 request every 10 seconds.
+     * make sure the IP address is allowed to perfom a request
      *
      * @access public
      * @static
-     * @return bool
+     * @throws Exception
+     * @return true
      */
     public static function canPass()
     {
+        // if creators are defined, the traffic limiter will only allow creation
+        // for these, with no limits, and skip any other rules
+        if (!empty(self::$_creators)) {
+            $creatorIps = explode(',', self::$_creators);
+            foreach ($creatorIps as $ipRange) {
+                if (self::matchIp($ipRange) === true) {
+                    return true;
+                }
+            }
+            throw new Exception(I18n::_('Your IP is not authorized to create pastes.'));
+        }
+
         // disable limits if set to less then 1
         if (self::$_limit < 1) {
             return true;
         }
 
-        // Check if $_ipKey is exempted from ratelimiting
+        // check if $_ipKey is exempted from ratelimiting
         if (!empty(self::$_exempted)) {
             $exIp_array = explode(',', self::$_exempted);
             foreach ($exIp_array as $ipRange) {
@@ -175,6 +210,10 @@ class TrafficLimiter extends AbstractPersistence
         if (!self::$_store->setValue((string) $tl, 'traffic_limiter', $hash)) {
             error_log('failed to store the traffic limiter, it probably contains outdated information');
         }
-        return $result;
+        if ($result) return true;
+        throw new Exception(I18n::_(
+            'Please wait %d seconds between each post.',
+            self::$_limit
+        ));
     }
 }

tst/Persistence/TrafficLimiterTest.php

@@ -38,13 +38,21 @@ class TrafficLimiterTest extends PHPUnit_Framework_TestCase
         $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
         $this->assertTrue(TrafficLimiter::canPass(), 'first request may pass');
         sleep(1);
-        $this->assertFalse(TrafficLimiter::canPass(), 'second request is to fast, may not pass');
+        try {
+            $this->assertFalse(TrafficLimiter::canPass(), 'expected an exception');
+        } catch (Exception $e) {
+            $this->assertEquals($e->getMessage(), 'Please wait 4 seconds between each post.', 'second request is to fast, may not pass');
+        }
         sleep(4);
         $this->assertTrue(TrafficLimiter::canPass(), 'third request waited long enough and may pass');
         $_SERVER['REMOTE_ADDR'] = '2001:1620:2057:dead:beef::cafe:babe';
         $this->assertTrue(TrafficLimiter::canPass(), 'fourth request has different ip and may pass');
         $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
-        $this->assertFalse(TrafficLimiter::canPass(), 'fifth request is to fast, may not pass');
+        try {
+            $this->assertFalse(TrafficLimiter::canPass(), 'expected an exception');
+        } catch (Exception $e) {
+            $this->assertEquals($e->getMessage(), 'Please wait 4 seconds between each post.', 'fifth request is to fast, may not pass');
+        }
     }
 
     public function testTrafficLimitExempted()
@@ -52,7 +60,11 @@ class TrafficLimiterTest extends PHPUnit_Framework_TestCase
         TrafficLimiter::setExempted('1.2.3.4,10.10.10.0/24,2001:1620:2057::/48');
         $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
         $this->assertTrue(TrafficLimiter::canPass(), 'first request may pass');
-        $this->assertFalse(TrafficLimiter::canPass(), 'not exempted');
+        try {
+            $this->assertFalse(TrafficLimiter::canPass(), 'expected an exception');
+        } catch (Exception $e) {
+            $this->assertEquals($e->getMessage(), 'Please wait 4 seconds between each post.', 'not exempted');
+        }
         $_SERVER['REMOTE_ADDR'] = '10.10.10.10';
         $this->assertTrue(TrafficLimiter::canPass(), 'IPv4 in exempted range');
         $this->assertTrue(TrafficLimiter::canPass(), 'request is to fast, but IPv4 in exempted range');
@@ -61,7 +73,11 @@ class TrafficLimiterTest extends PHPUnit_Framework_TestCase
         $this->assertTrue(TrafficLimiter::canPass(), 'request is to fast, but IPv6 in exempted range');
         TrafficLimiter::setExempted('127.*,foobar');
         $this->assertTrue(TrafficLimiter::canPass(), 'first cached request may pass');
-        $this->assertFalse(TrafficLimiter::canPass(), 'request is to fast, invalid range');
+        try {
+            $this->assertFalse(TrafficLimiter::canPass(), 'expected an exception');
+        } catch (Exception $e) {
+            $this->assertEquals($e->getMessage(), 'Please wait 4 seconds between each post.', 'request is to fast, invalid range');
+        }
         $_SERVER['REMOTE_ADDR'] = 'foobar';
         $this->assertTrue(TrafficLimiter::canPass(), 'non-IP address');
         $this->assertTrue(TrafficLimiter::canPass(), 'request is to fast, but non-IP address matches exempted range');
@@ -71,16 +87,23 @@ class TrafficLimiterTest extends PHPUnit_Framework_TestCase
     {
         TrafficLimiter::setCreators('1.2.3.4,10.10.10.0/24,2001:1620:2057::/48');
         $_SERVER['REMOTE_ADDR'] = '127.0.0.1';
-        $this->assertFalse(TrafficLimiter::canPass(), 'not a creator');
+        try {
+            $this->assertFalse(TrafficLimiter::canPass(), 'expected an exception');
+        } catch (Exception $e) {
+            $this->assertEquals($e->getMessage(), 'Your IP is not authorized to create pastes.', 'not a creator');
+        }
         $_SERVER['REMOTE_ADDR'] = '10.10.10.10';
         $this->assertTrue(TrafficLimiter::canPass(), 'IPv4 in creator range');
         $this->assertTrue(TrafficLimiter::canPass(), 'request is to fast, but IPv4 in creator range');
         $_SERVER['REMOTE_ADDR'] = '2001:1620:2057:dead:beef::cafe:babe';
         $this->assertTrue(TrafficLimiter::canPass(), 'IPv6 in creator range');
         $this->assertTrue(TrafficLimiter::canPass(), 'request is to fast, but IPv6 in creator range');
-        TrafficLimiter::setExempted('127.*,foobar');
-        $this->assertTrue(TrafficLimiter::canPass(), 'first cached request may pass');
-        $this->assertFalse(TrafficLimiter::canPass(), 'request is to fast, not a creator');
+        TrafficLimiter::setCreators('127.*,foobar');
+        try {
+            $this->assertFalse(TrafficLimiter::canPass(), 'expected an exception');
+        } catch (Exception $e) {
+            $this->assertEquals($e->getMessage(), 'Your IP is not authorized to create pastes.', 'request is to fast, not a creator');
+        }
         $_SERVER['REMOTE_ADDR'] = 'foobar';
         $this->assertTrue(TrafficLimiter::canPass(), 'non-IP address');
         $this->assertTrue(TrafficLimiter::canPass(), 'request is to fast, but non-IP address matches creator');