Refs #534

 * To ensure that the "label filtering" works on all types of labels including special characters "' <> etc.  I refactored the SafeDecodeLabel filter function. 
 * Changed DataTableLabelFilter to use Piwik_API_ResponseBuilder directly rather than duplicating code
 * Added few tests cases 



git-svn-id: http://dev.piwik.org/svn/trunk@5774 59fd770c-687e-43c8-a1e3-f5a4ff64c105

core/API/DataTableLabelFilter.php

@@ -29,7 +29,7 @@ class Piwik_API_DataTableLabelFilter
 {
     
     /** The separator to be used for specifying recursive labels */
-    const RECURSIVE_LABEL_SEPARATOR = '->>-';
+    const RECURSIVE_LABEL_SEPARATOR = '->>-';
 	
 	private $apiModule;
 	private $apiMethod;
@@ -133,51 +133,55 @@ class Piwik_API_DataTableLabelFilter
 	 */
 	protected function doFilterRecursiveDescend($labelParts, $dataTable, $date=false)
 	{
-		if ($dataTable instanceof Piwik_DataTable)
+		if(!($dataTable instanceof Piwik_DataTable))
 		{
-			// search for the first part of the tree search
-            $labelPart = array_shift($labelParts);
-            $row = $dataTable->getRowFromLabel($labelPart);
-			if ($row === false)
-			{
-				$labelPart = htmlentities($labelPart);
-				$row = $dataTable->getRowFromLabel($labelPart);
-			}
-			if ($row === false)
-			{
-				// not found
-				return false;
-			}
-			
-			// end of tree search reached
-			if (count($labelParts) == 0)
-			{
-				return $row;
-			}
-			
-			// match found on this level and more levels remaining: go deeper
-			$request = $this->request;
-            
-            // this is why the filter does not work with expanded=1:
-            // if the entire table is loaded, the id of sub-datatable has a different semantic.
-            $idSubTable = $row->getIdSubDataTable();
-            
-			$request['idSubtable'] = $idSubTable;
-			if ($date)
-			{
-				$request['date'] = $date;
-			}
-			
-			$class = 'Piwik_'.$this->apiModule.'_API';
-            $method = $this->getApiMethodForSubtable();
+			throw new Exception("Using the label filter is not supported for DataTable ".get_class($dataTable));
+		}
+		// search for the first part of the tree search
+        $labelPart = array_shift($labelParts);
+        $row = $dataTable->getRowFromLabel($labelPart);
+		if ($row === false)
+		{
+			$labelPart = htmlentities($labelPart);
+			$row = $dataTable->getRowFromLabel($labelPart);
+		}
+		if ($row === false)
+		{
+			// not found
+			return false;
+		}
+
+		// end of tree search reached
+		if (count($labelParts) == 0)
+		{
+			return $row;
+		}
+
+		// match found on this level and more levels remaining: go deeper
+		$request = $this->request;
             
-			$dataTable = Piwik_API_Proxy::getInstance()->call($class, $method, $request);
-            $dataTable->applyQueuedFilters();
+        // this is why the filter does not work with expanded=1:
+        // if the entire table is loaded, the id of sub-datatable has a different semantic.
+        $idSubTable = $row->getIdSubDataTable();
             
-			return $this->doFilterRecursiveDescend($labelParts, $dataTable, $date);
+		$request['idSubtable'] = $idSubTable;
+		if ($date)
+		{
+			$request['date'] = $date;
 		}
-
-		throw new Exception("Using the label filter is not supported for DataTable ".get_class($dataTable));
+			
+		$class = 'Piwik_'.$this->apiModule.'_API';
+        $method = $this->getApiMethodForSubtable();
+        
+        // Clean up request for Piwik_API_ResponseBuilder to behave correctly
+        unset($request['label']);
+        $request['serialize'] = 0;
+        
+		$dataTable = Piwik_API_Proxy::getInstance()->call($class, $method, $request);
+		$response = new Piwik_API_ResponseBuilder($format = 'original', $request);
+		$dataTable = $response->getResponse($dataTable);
+		
+		return $this->doFilterRecursiveDescend($labelParts, $dataTable, $date);
 	}
 	
 	private function getApiMethodForSubtable()
@@ -194,7 +198,6 @@ class Piwik_API_DataTableLabelFilter
 				$this->apiMethodForSubtable = $this->apiMethod;
 			}
 		}
-		
 		return $this->apiMethodForSubtable; 
 	}
 	

core/API/ResponseBuilder.php

@@ -283,7 +283,7 @@ class Piwik_API_ResponseBuilder
 			$genericFilter = new Piwik_API_DataTableGenericFilter($this->request);
 			$genericFilter->filter($datatable);
 		}
-		
+        
 		// we automatically safe decode all datatable labels (against xss) 
 		$datatable->queueFilter('SafeDecodeLabel');
         
@@ -292,16 +292,16 @@ class Piwik_API_ResponseBuilder
 		{
 			$datatable->applyQueuedFilters();
 		}
-        
+	
         // apply label filter: only return a single row matching the label parameter
         $label = Piwik_Common::getRequestVar('label', '', 'string', $this->request);
-        if ($label != '')
+        if ($label !== '')
         {
-            $label = html_entity_decode($label);
+            $label = Piwik_Common::unsanitizeInputValue($label);
+            $label = Piwik_DataTable_Filter_SafeDecodeLabel::filterValue($label);
             $filter = new Piwik_API_DataTableLabelFilter;
             $datatable = $filter->filter($label, $datatable, $this->apiModule, $this->apiMethod, $this->request);
         }
-        
 		return $this->getRenderedDataTable($datatable);
 	}
 	

core/DataTable.php

@@ -482,7 +482,6 @@ class Piwik_DataTable
 		if ($row !== false)
 		{
 			$newTable->addRow($row);
-			
 		}
 		return $newTable;
 	}

core/DataTable/Filter/SafeDecodeLabel.php

@@ -17,12 +17,21 @@
 class Piwik_DataTable_Filter_SafeDecodeLabel extends Piwik_DataTable_Filter
 {
 	private $columnToDecode;
-	private $outputHtml;
+	static private $outputHtml = true;
 	public function __construct( $table )
 	{
 		parent::__construct($table);
 		$this->columnToDecode = 'label';
-		$this->outputHtml = true;
+	}
+	
+	static public function filterValue($value)
+	{
+		$value = htmlspecialchars_decode( urldecode($value), ENT_QUOTES);
+		if(self::$outputHtml)
+		{
+			$value = htmlspecialchars($value, ENT_QUOTES);
+		}
+		return $value;
 	}
 	
 	public function filter($table)
@@ -32,17 +41,12 @@ class Piwik_DataTable_Filter_SafeDecodeLabel extends Piwik_DataTable_Filter
 			$value = $row->getColumn($this->columnToDecode);
 			if($value !== false)
 			{
-				$value = htmlspecialchars_decode(
-										urldecode($value),
-										ENT_QUOTES);
-				if($this->outputHtml)
-				{
-					$value = htmlspecialchars($value, ENT_QUOTES);
-				}
+				$value = self::filterValue($value);
 				$row->setColumn($this->columnToDecode,$value);
-			
+				
 				$this->filterSubTable($row);
 			}
 		}
 	}
+	
 }

plugins/Actions/Actions.php

@@ -674,7 +674,7 @@ class Piwik_Actions extends Piwik_Plugin
 		{
 			$name = $urlPath;
 			
-			if( empty($name) || substr($name, -1) == '/' )
+			if( $name === '' || substr($name, -1) == '/' )
 			{
 				$name .= self::$defaultActionName;
 			}

tests/integration/LabelFilter.test.php

@@ -14,20 +14,27 @@ class Test_Piwik_Integration_LabelFilter extends Test_Integration_Facade
 {
 	protected $dateTime = '2010-03-06 11:22:33';
 	protected $idSite = null;
-
+	
 	public function getApiToTest()
 	{
         $labelsToTest = array(
             // first level
-            'nonExistent', 'dir', '/this is cool!',
+            'nonExistent', 
+            'dir', 
+            '/0',
+            '/ééé"\'... <this is cool>!',
+            
             // second level
-            'dir->>-nonExistent', 'dir->>-/file.php?foo=bar&foo2=bar',
-            // third level
-            'dir2->>-sub->>-/file.php'
+            'dir->>-nonExistent', 
+            'dir->>-/file.php?foo=bar&foo2=bar',
+			
+            // 4 levels
+            'dir2->>-sub->>-0->>-/file.php'
         );
         
         $return = array();
         foreach ($labelsToTest as $label) {
+//        	var_dump(urlencode($label));
             $return[] = array('Actions.getPageUrls', array(
                 'testSuffix' => '_'.preg_replace('/[^a-z0-9]*/mi', '', $label),
                 'idSite' => $this->idSite,
@@ -76,7 +83,7 @@ class Test_Piwik_Integration_LabelFilter extends Test_Integration_Facade
     	$idSite = $this->idSite;
         $t = $this->getTracker($idSite, $dateTime, $defaultInit = true, $useThirdPartyCookie = 1);
         
-        $t->setUrl('http://example.org/this%20is%20cool!');
+        $t->setUrl('http://example.org/%C3%A9%C3%A9%C3%A9%22%27...%20%3Cthis%20is%20cool%3E!');
         $this->checkResponse($t->doTrackPageView('incredible title!'));
         
         $t->setUrl('http://example.org/dir/file.php?foo=bar&foo2=bar');
@@ -91,9 +98,14 @@ class Test_Piwik_Integration_LabelFilter extends Test_Integration_Facade
         $t->setForceVisitDateTime(Piwik_Date::factory($dateTime)->addHour(0.4)->getDatetime());
         $this->checkResponse($t->doTrackPageView('incredible title!'));
         
-        $t->setUrl('http://example.org/dir2/sub/file.php');
+        $t->setUrl('http://example.org/dir2/sub/0/file.php');
         $t->setForceVisitDateTime(Piwik_Date::factory($dateTime)->addHour(0.4)->getDatetime());
         $this->checkResponse($t->doTrackPageView('incredible title!'));
+        
+        $t->setUrl('http://example.org/0');
+        $t->setForceVisitDateTime(Piwik_Date::factory($dateTime)->addHour(0.4)->getDatetime());
+        $this->checkResponse($t->doTrackPageView('I am URL zero!'));
+        
 	}
 }
 

tests/integration/expected/test_LabelFilter_dir2subfilephp__Actions.getPageUrls_day.xml → tests/integration/expected/test_LabelFilter_0__Actions.getPageUrls_day.xml

 <?xml version="1.0" encoding="utf-8" ?>
 <result>
 	<row>
-		<label>/file.php</label>
+		<label>/0</label>
 		<nb_visits>1</nb_visits>
 		<nb_uniq_visitors>1</nb_uniq_visitors>
 		<nb_hits>1</nb_hits>
@@ -11,6 +11,6 @@
 		<avg_time_on_page>0</avg_time_on_page>
 		<bounce_rate>0%</bounce_rate>
 		<exit_rate>100%</exit_rate>
-		<url>http://example.org/dir2/sub/file.php</url>
+		<url>http://example.org/0</url>
 	</row>
 </result>
\ No newline at end of file

tests/integration/expected/test_LabelFilter_dir2sub0filephp__Actions.getPageUrls_day.xml

+<?xml version="1.0" encoding="utf-8" ?>
+<result>
+	<row>
+		<label>/file.php</label>
+		<nb_visits>1</nb_visits>
+		<nb_uniq_visitors>1</nb_uniq_visitors>
+		<nb_hits>1</nb_hits>
+		<sum_time_spent>0</sum_time_spent>
+		<avg_time_on_page>0</avg_time_on_page>
+		<bounce_rate>0%</bounce_rate>
+		<exit_rate>0%</exit_rate>
+		<url>http://example.org/dir2/sub/0/file.php</url>
+	</row>
+</result>
\ No newline at end of file

tests/integration/expected/test_LabelFilter_thisiscool__Actions.getPageUrls_day.xml

 <?xml version="1.0" encoding="utf-8" ?>
 <result>
 	<row>
-		<label>/this is cool!</label>
+		<label>/ééé&quot;&amp;#039;... &lt;this is cool&gt;!</label>
 		<nb_visits>1</nb_visits>
 		<nb_uniq_visitors>1</nb_uniq_visitors>
 		<nb_hits>1</nb_hits>
 		<sum_time_spent>720</sum_time_spent>
 		<entry_nb_uniq_visitors>1</entry_nb_uniq_visitors>
 		<entry_nb_visits>1</entry_nb_visits>
-		<entry_nb_actions>5</entry_nb_actions>
+		<entry_nb_actions>6</entry_nb_actions>
 		<entry_sum_visit_length>1441</entry_sum_visit_length>
 		<entry_bounce_count>0</entry_bounce_count>
 		<avg_time_on_page>720</avg_time_on_page>
 		<bounce_rate>0%</bounce_rate>
 		<exit_rate>0%</exit_rate>
-		<url>http://example.org/this%20is%20cool!</url>
+		<url>http://example.org/%C3%A9%C3%A9%C3%A9%22%27...%20%3Cthis%20is%20cool%3E!</url>
 	</row>
 </result>
\ No newline at end of file