From 241f0604b00f689fca9255666b323906e5e7b314 Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 30 Apr 2014 12:31:18 +1200 Subject: [PATCH] Namespaced filters Changed default settings (disable lock_bypage and set lock_timeout to 5) Updated docs --- _config/versionfeed.yml | 4 ++-- code/VersionFeed_Controller.php | 6 +++--- code/caching/CachedContentFilter.php | 2 ++ code/caching/ContentFilter.php | 4 +++- code/caching/RateLimitFilter.php | 18 +++++++++--------- docs/en/developer.md | 18 ++++++++---------- tests/VersionFeedFunctionalTest.php | 24 ++++++++++++------------ 7 files changed, 39 insertions(+), 37 deletions(-) diff --git a/_config/versionfeed.yml b/_config/versionfeed.yml index 1bcf83e..e6e99eb 100644 --- a/_config/versionfeed.yml +++ b/_config/versionfeed.yml @@ -2,9 +2,9 @@ Name: versionedfeedconfig --- Injector: - RateLimitFilter: RateLimitFilter + RateLimitFilter: \VersionFeed\Filters\RateLimitFilter ContentFilter: - class: CachedContentFilter + class: \VersionFeed\Filters\CachedContentFilter constructor: - %$RateLimitFilter SiteTree: diff --git a/code/VersionFeed_Controller.php b/code/VersionFeed_Controller.php index a59507a..e39b476 100644 --- a/code/VersionFeed_Controller.php +++ b/code/VersionFeed_Controller.php @@ -10,16 +10,16 @@ class VersionFeed_Controller extends Extension { /** * Content handler * - * @var ContentFilter + * @var \VersionFeed\Filters\ContentFilter */ protected $contentFilter; /** * Sets the content filter * - * @param ContentFilter $contentFilter + * @param \VersionFeed\Filters\ContentFilter $contentFilter */ - public function setContentFilter(ContentFilter $contentFilter) { + public function setContentFilter(\VersionFeed\Filters\ContentFilter $contentFilter) { $this->contentFilter = $contentFilter; } diff --git a/code/caching/CachedContentFilter.php b/code/caching/CachedContentFilter.php index 7173379..d3b1f23 100644 --- a/code/caching/CachedContentFilter.php +++ b/code/caching/CachedContentFilter.php @@ -1,5 +1,7 @@ setOption('automatic_serialization', true); return $cache; } diff --git a/code/caching/RateLimitFilter.php b/code/caching/RateLimitFilter.php index 181d2af..39663c7 100644 --- a/code/caching/RateLimitFilter.php +++ b/code/caching/RateLimitFilter.php @@ -1,5 +1,7 @@ get(get_class(), 'lock_bypage')) { + if(\Config::inst()->get(get_class(), 'lock_bypage')) { $key .= '_' . md5($itemkey); } // Add user-specific identifier - if(Config::inst()->get(get_class(), 'lock_byuserip') && Controller::has_curr()) { - $ip = Controller::curr()->getRequest()->getIP(); + if(\Config::inst()->get(get_class(), 'lock_byuserip') && \Controller::has_curr()) { + $ip = \Controller::curr()->getRequest()->getIP(); $key .= '_' . md5($ip); } @@ -66,7 +66,7 @@ class RateLimitFilter extends ContentFilter { public function getContent($key, $callback) { // Bypass rate limiting if flushing, or timeout isn't set - $timeout = Config::inst()->get(get_class(), 'lock_timeout'); + $timeout = \Config::inst()->get(get_class(), 'lock_timeout'); if(isset($_GET['flush']) || !$timeout) { return parent::getContent($key, $callback); } @@ -77,7 +77,7 @@ class RateLimitFilter extends ContentFilter { if($cacheBegin = $cache->load($limitKey)) { if(time() - $cacheBegin < $timeout) { // Politely inform visitor of limit - $response = new SS_HTTPResponse_Exception('Too Many Requests.', 429); + $response = new \SS_HTTPResponse_Exception('Too Many Requests.', 429); $response->getResponse()->addHeader('Retry-After', 1 + time() - $cacheBegin); throw $response; } diff --git a/docs/en/developer.md b/docs/en/developer.md index 0173656..5d4ee74 100644 --- a/docs/en/developer.md +++ b/docs/en/developer.md @@ -36,20 +36,18 @@ Two filters are applied on top of one another: Either one of these can be replaced, added to, or removed, by adjusting the `VersionFeed_Controller.dependencies` config to point to a replacement (or no) filter. -For large servers with a high level of traffic (more than 1 visits every 10 seconds) the default settings should -be sufficient. +For smaller servers where it's reasonable to apply a strict approach to rate limiting the default +settings should be sufficient. The `RateLimitFilter.lock_bypage` config defaults to false, meaning that a +single limit will be applied to all URLs. If set to true, then each URL will have its own rate limit, +and on smaller servers with lots of concurrent requests this can still overwhelm capacity. This will +also leave smaller servers vulnerable to DDoS attacks which target many URLs simultaneously. +This config will have no effect on the `allchanges` method. -For smaller servers where it's reasonable to apply a more strict approach to rate limiting, consider setting the -`RateLimitFilter.lock_bypage` config to false, meaning that a single limit will be applied to all URLs. -If left on true, then each URL will have its own rate limit, and on smaller servers with lots of -concurrent requests this can still overwhelm capacity. This will also leave smaller servers vulnerable to DDoS -attacks which target many URLs simultaneously. This config will have no effect on the `allchanges` method. - -`RateLimitFilter.lock_byuserip` can also be set to true in order to prevent requests from different users +`RateLimitFilter.lock_byuserip` can be set to true in order to prevent requests from different users interfering with one another. However, this can provide an ineffective safeguard against malicious DDoS attacks which use multiple IP addresses. -Another important variable is the `RateLimitFilter.lock_timeout` config, which is set to 10 seconds by default. +Another important variable is the `RateLimitFilter.lock_timeout` config, which is set to 5 seconds by default. This should be increased on sites which may be slow to generate page versions, whether due to lower server capacity or volume of content (number of page versions). Requests to this page after the timeout will not trigger any rate limit safeguard, so you should be sure that this is set to an appropriate level. diff --git a/tests/VersionFeedFunctionalTest.php b/tests/VersionFeedFunctionalTest.php index bb8ea71..92b4d92 100644 --- a/tests/VersionFeedFunctionalTest.php +++ b/tests/VersionFeedFunctionalTest.php @@ -17,9 +17,9 @@ class VersionFeedFunctionalTest extends FunctionalTest { $this->userIP = isset($_SERVER['HTTP_CLIENT_IP']) ? $_SERVER['HTTP_CLIENT_IP'] : null; Config::nest(); - Config::inst()->update('RateLimitFilter', 'lock_timeout', 20); - Config::inst()->update('RateLimitFilter', 'lock_bypage', false); - Config::inst()->update('RateLimitFilter', 'lock_byuserip', false); + Config::inst()->update('VersionFeed\Filters\RateLimitFilter', 'lock_timeout', 20); + Config::inst()->update('VersionFeed\Filters\RateLimitFilter', 'lock_bypage', false); + Config::inst()->update('VersionFeed\Filters\RateLimitFilter', 'lock_byuserip', false); } public function tearDown() { @@ -60,10 +60,10 @@ class VersionFeedFunctionalTest extends FunctionalTest { $page2 = $this->createPageWithChanges(array('PublicHistory' => true, 'Title' => 'Page2')); // Artifically set cache lock - Config::inst()->update('RateLimitFilter', 'lock_byuserip', false); + Config::inst()->update('VersionFeed\Filters\RateLimitFilter', 'lock_byuserip', false); $cache = SS_Cache::factory('VersionFeed_Controller'); $cache->setOption('automatic_serialization', true); - $cache->save(time() - 2, RateLimitFilter::CACHE_PREFIX); + $cache->save(time() - 2, \VersionFeed\Filters\RateLimitFilter::CACHE_PREFIX); // Test normal hit $response = $this->get($page1->RelativeLink('changes')); @@ -74,37 +74,37 @@ class VersionFeedFunctionalTest extends FunctionalTest { $this->assertGreaterThan(0, $response->getHeader('Retry-After')); // Test page specific lock - Config::inst()->update('RateLimitFilter', 'lock_bypage', true); + Config::inst()->update('VersionFeed\Filters\RateLimitFilter', 'lock_bypage', true); $key = implode('_', array( 'changes', $page1->ID, Versioned::get_versionnumber_by_stage('SiteTree', 'Live', $page1->ID, false) )); - $key = RateLimitFilter::CACHE_PREFIX . '_' . md5($key); + $key = \VersionFeed\Filters\RateLimitFilter::CACHE_PREFIX . '_' . md5($key); $cache->save(time() - 2, $key); $response = $this->get($page1->RelativeLink('changes')); $this->assertEquals(429, $response->getStatusCode()); $this->assertGreaterThan(0, $response->getHeader('Retry-After')); $response = $this->get($page2->RelativeLink('changes')); $this->assertEquals(200, $response->getStatusCode()); - Config::inst()->update('RateLimitFilter', 'lock_bypage', false); + Config::inst()->update('VersionFeed\Filters\RateLimitFilter', 'lock_bypage', false); // Test rate limit hit by IP - Config::inst()->update('RateLimitFilter', 'lock_byuserip', true); + Config::inst()->update('VersionFeed\Filters\RateLimitFilter', 'lock_byuserip', true); $_SERVER['HTTP_CLIENT_IP'] = '127.0.0.1'; - $cache->save(time() - 2, RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1')); + $cache->save(time() - 2, \VersionFeed\Filters\RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1')); $response = $this->get($page1->RelativeLink('changes')); $this->assertEquals(429, $response->getStatusCode()); $this->assertGreaterThan(0, $response->getHeader('Retry-After')); // Test rate limit doesn't hit other IP $_SERVER['HTTP_CLIENT_IP'] = '127.0.0.20'; - $cache->save(time() - 2, RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1')); + $cache->save(time() - 2, \VersionFeed\Filters\RateLimitFilter::CACHE_PREFIX . '_' . md5('127.0.0.1')); $response = $this->get($page1->RelativeLink('changes')); $this->assertEquals(200, $response->getStatusCode()); // Restore setting - Config::inst()->update('RateLimitFilter', 'lock_byuserip', false); + Config::inst()->update('VersionFeed\Filters\RateLimitFilter', 'lock_byuserip', false); } public function testContainsChangesForPageOnly() {