fix XSS when importing articles, speed up update and adding of feeds by only purifying content that will be added to the db

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&amp;b=2">'))
-			->will($this->returnValue('<img src="https://username:secret@www.explosm.net/all/relative/url.png?a=1&amp;b=2">'));
 
 		$result = $this->testEnhancer->enhance($item);
 		$this->assertEquals('<img src="https://username:secret@www.explosm.net/all/relative/url.png?a=1&amp;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);