diff options
author | Bernhard Posselt <dev@bernhard-posselt.com> | 2014-02-11 16:05:37 +0100 |
---|---|---|
committer | Bernhard Posselt <dev@bernhard-posselt.com> | 2014-02-11 16:05:58 +0100 |
commit | 99af7d32d42d7b77bae4f7747c02db959f35d668 (patch) | |
tree | 97e9141c60fb21e7e38b785d0b278da7e31792ca | |
parent | d5eab3852c1c6629be6b29016e67e374d85f78ac (diff) |
fix XSS when importing articles, speed up update and adding of feeds by only purifying content that will be added to the db
-rw-r--r-- | articleenhancer/xpatharticleenhancer.php | 11 | ||||
-rw-r--r-- | businesslayer/feedbusinesslayer.php | 8 | ||||
-rw-r--r-- | dependencyinjection/dicontainer.php | 7 | ||||
-rw-r--r-- | fetcher/feedfetcher.php | 15 | ||||
-rw-r--r-- | tests/unit/articleenhancer/XPathArticleEnhancerTest.php | 35 | ||||
-rw-r--r-- | tests/unit/businesslayer/FeedBusinessLayerTest.php | 28 | ||||
-rw-r--r-- | tests/unit/fetcher/FeedFetcherTest.php | 8 |
7 files changed, 45 insertions, 67 deletions
diff --git a/articleenhancer/xpatharticleenhancer.php b/articleenhancer/xpatharticleenhancer.php index 0a2d6e56c..6cc11eb65 100644 --- a/articleenhancer/xpatharticleenhancer.php +++ b/articleenhancer/xpatharticleenhancer.php @@ -33,23 +33,19 @@ class XPathArticleEnhancer implements ArticleEnhancer { private $feedRegex; - private $purifier; private $fileFactory; private $maximumTimeout; /** - * @param $purifier the purifier object to clean the html which will be - * matched * @param SimplePieFileFactory a factory for getting a simple pie file instance * @param array $regexXPathPair an associative array containing regex to * match the url and the xpath that should be used for it to extract the * page * @param int $maximumTimeout maximum timeout in seconds, defaults to 10 sec */ - public function __construct($purifier, SimplePieFileFactory $fileFactory, + public function __construct(SimplePieFileFactory $fileFactory, array $regexXPathPair, $maximumTimeout=10){ - $this->purifier = $purifier; $this->regexXPathPair = $regexXPathPair; $this->fileFactory = $fileFactory; $this->maximumTimeout = $maximumTimeout; @@ -85,9 +81,8 @@ class XPathArticleEnhancer implements ArticleEnhancer { // convert all relative to absolute URLs $xpathResult = $this->substituteRelativeLinks($xpathResult, $item->getUrl()); - $sanitizedResult = $this->purifier->purify($xpathResult); - if( $sanitizedResult ) { - $item->setBody($sanitizedResult); + if( $xpathResult ) { + $item->setBody($xpathResult); } } } diff --git a/businesslayer/feedbusinesslayer.php b/businesslayer/feedbusinesslayer.php index 58a4ed578..c9b993e5a 100644 --- a/businesslayer/feedbusinesslayer.php +++ b/businesslayer/feedbusinesslayer.php @@ -45,12 +45,14 @@ class FeedBusinessLayer extends BusinessLayer { private $timeFactory; private $autoPurgeMinimumInterval; private $enhancer; + private $purifier; public function __construct(FeedMapper $feedMapper, Fetcher $feedFetcher, ItemMapper $itemMapper, API $api, TimeFactory $timeFactory, $autoPurgeMinimumInterval, - Enhancer $enhancer){ + Enhancer $enhancer, + $purifier){ parent::__construct($feedMapper); $this->feedFetcher = $feedFetcher; $this->itemMapper = $itemMapper; @@ -58,6 +60,7 @@ class FeedBusinessLayer extends BusinessLayer { $this->timeFactory = $timeFactory; $this->autoPurgeMinimumInterval = $autoPurgeMinimumInterval; $this->enhancer = $enhancer; + $this->purifier = $purifier; } /** @@ -122,6 +125,7 @@ class FeedBusinessLayer extends BusinessLayer { } catch(DoesNotExistException $ex){ $unreadCount += 1; $item = $this->enhancer->enhance($item, $feed->getLink()); + $item->setBody($this->purifier->purify($item->getBody())); $this->itemMapper->insert($item); } } @@ -192,6 +196,7 @@ class FeedBusinessLayer extends BusinessLayer { } catch(DoesNotExistException $ex){ $item = $this->enhancer->enhance($item, $existingFeed->getLink()); + $item->setBody($this->purifier->purify($item->getBody())); $this->itemMapper->insert($item); } } @@ -294,6 +299,7 @@ class FeedBusinessLayer extends BusinessLayer { $existingItem->setStatus($item->getStatus()); $this->itemMapper->update($existingItem); } catch(DoesNotExistException $ex){ + $item->setBody($this->purifier->purify($item->getBody())); $this->itemMapper->insert($item); } } diff --git a/dependencyinjection/dicontainer.php b/dependencyinjection/dicontainer.php index 480635d8e..d8e286bd6 100644 --- a/dependencyinjection/dicontainer.php +++ b/dependencyinjection/dicontainer.php @@ -194,7 +194,8 @@ class DIContainer extends BaseContainer { $c['API'], $c['TimeFactory'], $c['autoPurgeMinimumInterval'], - $c['Enhancer']); + $c['Enhancer'], + $c['HTMLPurifier']); }); $this['ItemBusinessLayer'] = $this->share(function($c){ @@ -264,7 +265,6 @@ class DIContainer extends BaseContainer { foreach(json_decode($xpathEnhancerConfig, true) as $feed => $config) { $articleEnhancer = new XPathArticleEnhancer( - $c['HTMLPurifier'], $c['SimplePieFileFactory'], $config, $c['feedFetcherTimeout'] @@ -303,8 +303,7 @@ class DIContainer extends BaseContainer { $c['TimeFactory'], $c['simplePieCacheDirectory'], $c['simplePieCacheDuration'], - $c['feedFetcherTimeout'], - $c['HTMLPurifier']); + $c['feedFetcherTimeout']); }); $this['StatusFlag'] = $this->share(function($c){ diff --git a/fetcher/feedfetcher.php b/fetcher/feedfetcher.php index fc9f4eae6..aa1f94e5b 100644 --- a/fetcher/feedfetcher.php +++ b/fetcher/feedfetcher.php @@ -42,8 +42,7 @@ class FeedFetcher implements IFeedFetcher { private $faviconFetcher; private $simplePieFactory; private $fetchTimeout; - private $time; - private $purifier; + private $time; public function __construct(API $api, SimplePieAPIFactory $simplePieFactory, @@ -51,15 +50,13 @@ class FeedFetcher implements IFeedFetcher { TimeFactory $time, $cacheDirectory, $cacheDuration, - $fetchTimeout, - $purifier){ + $fetchTimeout){ $this->api = $api; $this->cacheDirectory = $cacheDirectory; $this->cacheDuration = $cacheDuration; $this->faviconFetcher = $faviconFetcher; $this->simplePieFactory = $simplePieFactory; $this->time = $time; - $this->purifier = $purifier; $this->fetchTimeout = $fetchTimeout; } @@ -143,12 +140,8 @@ class FeedFetcher implements IFeedFetcher { $guid = $simplePieItem->get_id(); $item->setGuid($guid); - // links should always open in a new window - $item->setBody( - $this->purifier->purify( - $simplePieItem->get_content() - ) - ); + // purification is done in the businesslayer + $item->setBody($simplePieItem->get_content()); // pubdate is not required. if not given use the current date $date = $simplePieItem->get_date('U'); diff --git a/tests/unit/articleenhancer/XPathArticleEnhancerTest.php b/tests/unit/articleenhancer/XPathArticleEnhancerTest.php index 798fa9203..60b8e0a90 100644 --- a/tests/unit/articleenhancer/XPathArticleEnhancerTest.php +++ b/tests/unit/articleenhancer/XPathArticleEnhancerTest.php @@ -32,7 +32,6 @@ require_once(__DIR__ . "/../../classloader.php"); class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { - private $purifier; private $testEnhancer; private $fileFactory; private $timeout; @@ -42,10 +41,8 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { $this->fileFactory = $this->getMockBuilder('\OCA\News\Utility\SimplePieFileFactory') ->disableOriginalConstructor() ->getMock(); - $this->purifier = $this->getMock('purifier', array('purify')); $this->testEnhancer = new XPathArticleEnhancer( - $this->purifier, $this->fileFactory, array( '/explosm.net\/comics/' => '//*[@id=\'maincontent\']/div[2]/div/span', @@ -85,10 +82,6 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($item->getUrl()), $this->equalTo($this->timeout)) ->will($this->returnValue($file)); - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo('<span>hiho</span>')) - ->will($this->returnValue('<span>hiho</span>')); $result = $this->testEnhancer->enhance($item); $this->assertEquals('<span>hiho</span>', $result->getBody()); @@ -115,10 +108,6 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($item->getUrl()), $this->equalTo($this->timeout)) ->will($this->returnValue($file)); - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo('<div>hiho</div><div>rawr</div>')) - ->will($this->returnValue('<div>hiho</div><div>rawr</div>')); $result = $this->testEnhancer->enhance($item); $this->assertEquals('<div>hiho</div><div>rawr</div>', $result->getBody()); @@ -143,10 +132,6 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($item->getUrl()), $this->equalTo($this->timeout)) ->will($this->returnValue($file)); - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo(null)) - ->will($this->returnValue(null)); $result = $this->testEnhancer->enhance($item); $this->assertEquals('Hello thar', $result->getBody()); @@ -166,10 +151,6 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($item->getUrl()), $this->equalTo($this->timeout)) ->will($this->returnValue($file)); - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo(null)) - ->will($this->returnValue(null)); $result = $this->testEnhancer->enhance($item); $this->assertEquals('Hello thar', $result->getBody()); @@ -194,10 +175,6 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($item->getUrl()), $this->equalTo($this->timeout)) ->will($this->returnValue($file)); - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo(null)) - ->will($this->returnValue(null)); $result = $this->testEnhancer->enhance($item); $this->assertEquals('Hello thar', $result->getBody()); @@ -223,10 +200,6 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($item->getUrl()), $this->equalTo($this->timeout)) ->will($this->returnValue($file)); - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo('<a href="https://www.explosm.net/a/relative/url.html?a=1#b">link</a><a href="https://www.explosm.net/all/b/relative/url.html">link2</a><img src="https://www.explosm.net/another/relative/link.jpg">')) - ->will($this->returnValue('<a href="https://www.explosm.net/a/relative/url.html?a=1#b">link</a><a href="https://www.explosm.net/all/b/relative/url.html">link2</a><img src="https://www.explosm.net/another/relative/link.jpg">')); $result = $this->testEnhancer->enhance($item); $this->assertEquals('<a target="_blank" href="https://www.explosm.net/a/relative/url.html?a=1#b">link</a><a target="_blank" href="https://www.explosm.net/all/b/relative/url.html">link2</a><img src="https://www.explosm.net/another/relative/link.jpg">', $result->getBody()); @@ -249,10 +222,6 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($item->getUrl()), $this->equalTo($this->timeout)) ->will($this->returnValue($file)); - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo('<img src="https://username:secret@www.explosm.net/all/relative/url.png?a=1&b=2">')) - ->will($this->returnValue('<img src="https://username:secret@www.explosm.net/all/relative/url.png?a=1&b=2">')); $result = $this->testEnhancer->enhance($item); $this->assertEquals('<img src="https://username:secret@www.explosm.net/all/relative/url.png?a=1&b=2">', $result->getBody()); @@ -276,10 +245,6 @@ class XPathArticleEnhancerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($item->getUrl()), $this->equalTo($this->timeout)) ->will($this->returnValue($file)); - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo('<img src="http://www.url.com/absolute/url.png"><a href="mailto:test@testsite.com">mail</a>')) - ->will($this->returnValue('<img src="http://www.url.com/absolute/url.png"><a href="mailto:test@testsite.com">mail</a>')); $result = $this->testEnhancer->enhance($item); $this->assertEquals('<img src="http://www.url.com/absolute/url.png"><a target="_blank" href="mailto:test@testsite.com">mail</a>', $result->getBody()); diff --git a/tests/unit/businesslayer/FeedBusinessLayerTest.php b/tests/unit/businesslayer/FeedBusinessLayerTest.php index 454a4966e..357ee73eb 100644 --- a/tests/unit/businesslayer/FeedBusinessLayerTest.php +++ b/tests/unit/businesslayer/FeedBusinessLayerTest.php @@ -48,6 +48,7 @@ class FeedBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility { private $importParser; private $autoPurgeMinimumInterval; private $enhancer; + private $purifier; protected function setUp(){ $this->api = $this->getAPIMock(); @@ -72,10 +73,11 @@ class FeedBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility { $this->enhancer = $this->getMockBuilder('\OCA\News\ArticleEnhancer\Enhancer') ->disableOriginalConstructor() ->getMock(); + $this->purifier = $this->getMock('purifier', array('purify')); $this->feedBusinessLayer = new FeedBusinessLayer($this->feedMapper, $this->fetcher, $this->itemMapper, $this->api, $timeFactory, $this->autoPurgeMinimumInterval, - $this->enhancer); + $this->enhancer, $this->purifier); $this->user = 'jack'; $response = 'hi'; } @@ -150,6 +152,10 @@ class FeedBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($return[1][1]), $this->equalTo($url)) ->will($this->returnValue($return[1][1])); + $this->purifier->expects($this->at(0)) + ->method('purify') + ->with($this->equalTo($return[1][1]->getBody())) + ->will($this->returnValue($return[1][1]->getBody())); $this->itemMapper->expects($this->at(1)) ->method('insert') ->with($this->equalTo($return[1][1])); @@ -165,6 +171,10 @@ class FeedBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($return[1][0]), $this->equalTo($url)) ->will($this->returnValue($return[1][0])); + $this->purifier->expects($this->at(1)) + ->method('purify') + ->with($this->equalTo($return[1][0]->getBody())) + ->will($this->returnValue($return[1][0]->getBody())); $this->itemMapper->expects($this->at(3)) ->method('insert') ->with($this->equalTo($return[1][0])); @@ -219,6 +229,10 @@ class FeedBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($return[1][1]), $this->equalTo($url)) ->will($this->returnValue($return[1][1])); + $this->purifier->expects($this->at(0)) + ->method('purify') + ->with($this->equalTo($return[1][1]->getBody())) + ->will($this->returnValue($return[1][1]->getBody())); $this->itemMapper->expects($this->at(1)) ->method('insert') ->with($this->equalTo($return[1][1])); @@ -274,6 +288,10 @@ class FeedBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility { ->with($this->equalTo($items[0]), $this->equalTo($feed->getUrl())) ->will($this->returnValue($items[0])); + $this->purifier->expects($this->at(0)) + ->method('purify') + ->with($this->equalTo($items[0]->getBody())) + ->will($this->returnValue($items[0]->getBody())); $this->itemMapper->expects($this->once()) ->method('insert') ->with($this->equalTo($items[0])); @@ -525,6 +543,10 @@ class FeedBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility { ->method('insert') ->with($this->equalTo($item)); + $this->purifier->expects($this->once()) + ->method('purify') + ->with($this->equalTo($item->getBody())) + ->will($this->returnValue($item->getBody())); $result = $this->feedBusinessLayer->importArticles($items, $this->user); @@ -595,6 +617,10 @@ class FeedBusinessLayerTest extends \OCA\AppFramework\Utility\TestUtility { $this->itemMapper->expects($this->at(0)) ->method('findByGuidHash') ->will($this->throwException(new DoesNotExistException('yo'))); + $this->purifier->expects($this->once()) + ->method('purify') + ->with($this->equalTo($item->getBody())) + ->will($this->returnValue($item->getBody())); $this->itemMapper->expects($this->at(1)) ->method('insert') ->with($this->equalTo($item)); diff --git a/tests/unit/fetcher/FeedFetcherTest.php b/tests/unit/fetcher/FeedFetcherTest.php index da7348fc9..b4ec42e60 100644 --- a/tests/unit/fetcher/FeedFetcherTest.php +++ b/tests/unit/fetcher/FeedFetcherTest.php @@ -81,7 +81,6 @@ class FeedFetcherTest extends \OCA\AppFramework\Utility\TestUtility { '\OCA\AppFramework\Utility\FaviconFetcher') ->disableOriginalConstructor() ->getMock(); - $this->purifier = $this->getMock('purifier', array('purify')); $this->time = 2323; $timeFactory = $this->getMockBuilder( '\OCA\AppFramework\Utility\TimeFactory') @@ -99,8 +98,7 @@ class FeedFetcherTest extends \OCA\AppFramework\Utility\TestUtility { $timeFactory, $this->cacheDirectory, $this->cacheDuration, - $this->fetchTimeout, - $this->purifier); + $this->fetchTimeout); $this->url = 'http://tests'; $this->permalink = 'http://permalink'; @@ -177,10 +175,6 @@ class FeedFetcherTest extends \OCA\AppFramework\Utility\TestUtility { private function createItem($author=false, $enclosureType=null, $noPubDate=false) { - $this->purifier->expects($this->once()) - ->method('purify') - ->with($this->equalTo($this->body)) - ->will($this->returnValue($this->body)); $this->expectItem('get_permalink', $this->permalink); $this->expectItem('get_title', $this->title); $this->expectItem('get_id', $this->guid); |