From ff6fa2ec964c19a21767240e204bfdc2e345d0dc Mon Sep 17 00:00:00 2001
From: mattpiwik <matthieu.aubry@gmail.com>
Date: Tue, 7 Feb 2012 21:15:51 +0000
Subject: [PATCH] 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 | 89 ++++++++++---------
core/API/ResponseBuilder.php | 10 +--
core/DataTable.php | 1 -
core/DataTable/Filter/SafeDecodeLabel.php | 24 ++---
plugins/Actions/Actions.php | 2 +-
tests/integration/LabelFilter.test.php | 26 ++++--
...abelFilter_0__Actions.getPageUrls_day.xml} | 4 +-
...r2sub0filephp__Actions.getPageUrls_day.xml | 14 +++
...er_thisiscool__Actions.getPageUrls_day.xml | 6 +-
9 files changed, 104 insertions(+), 72 deletions(-)
rename tests/integration/expected/{test_LabelFilter_dir2subfilephp__Actions.getPageUrls_day.xml => test_LabelFilter_0__Actions.getPageUrls_day.xml} (83%)
create mode 100644 tests/integration/expected/test_LabelFilter_dir2sub0filephp__Actions.getPageUrls_day.xml
diff --git a/core/API/DataTableLabelFilter.php b/core/API/DataTableLabelFilter.php
index bff0a34989..20f0cbd593 100644
--- a/core/API/DataTableLabelFilter.php
+++ b/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;
}
diff --git a/core/API/ResponseBuilder.php b/core/API/ResponseBuilder.php
index 1ac3959ab7..9b601286b9 100644
--- a/core/API/ResponseBuilder.php
+++ b/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);
}
diff --git a/core/DataTable.php b/core/DataTable.php
index b639976f40..01e16aecd4 100644
--- a/core/DataTable.php
+++ b/core/DataTable.php
@@ -482,7 +482,6 @@ class Piwik_DataTable
if ($row !== false)
{
$newTable->addRow($row);
-
}
return $newTable;
}
diff --git a/core/DataTable/Filter/SafeDecodeLabel.php b/core/DataTable/Filter/SafeDecodeLabel.php
index 128a14d894..381d3a9eb9 100644
--- a/core/DataTable/Filter/SafeDecodeLabel.php
+++ b/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);
}
}
}
+
}
diff --git a/plugins/Actions/Actions.php b/plugins/Actions/Actions.php
index 309c4c3629..08d3c942ad 100644
--- a/plugins/Actions/Actions.php
+++ b/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;
}
diff --git a/tests/integration/LabelFilter.test.php b/tests/integration/LabelFilter.test.php
index ea0fdd1a97..e66bcc6378 100644
--- a/tests/integration/LabelFilter.test.php
+++ b/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!'));
+
}
}
diff --git a/tests/integration/expected/test_LabelFilter_dir2subfilephp__Actions.getPageUrls_day.xml b/tests/integration/expected/test_LabelFilter_0__Actions.getPageUrls_day.xml
similarity index 83%
rename from tests/integration/expected/test_LabelFilter_dir2subfilephp__Actions.getPageUrls_day.xml
rename to tests/integration/expected/test_LabelFilter_0__Actions.getPageUrls_day.xml
index 2e0cf6314d..2af4405a51 100644
--- a/tests/integration/expected/test_LabelFilter_dir2subfilephp__Actions.getPageUrls_day.xml
+++ b/tests/integration/expected/test_LabelFilter_0__Actions.getPageUrls_day.xml
@@ -1,7 +1,7 @@
<?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
diff --git a/tests/integration/expected/test_LabelFilter_dir2sub0filephp__Actions.getPageUrls_day.xml b/tests/integration/expected/test_LabelFilter_dir2sub0filephp__Actions.getPageUrls_day.xml
new file mode 100644
index 0000000000..17728cab17
--- /dev/null
+++ b/tests/integration/expected/test_LabelFilter_dir2sub0filephp__Actions.getPageUrls_day.xml
@@ -0,0 +1,14 @@
+<?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
diff --git a/tests/integration/expected/test_LabelFilter_thisiscool__Actions.getPageUrls_day.xml b/tests/integration/expected/test_LabelFilter_thisiscool__Actions.getPageUrls_day.xml
index de3f93dcc9..8f42ec9bd2 100644
--- a/tests/integration/expected/test_LabelFilter_thisiscool__Actions.getPageUrls_day.xml
+++ b/tests/integration/expected/test_LabelFilter_thisiscool__Actions.getPageUrls_day.xml
@@ -1,19 +1,19 @@
<?xml version="1.0" encoding="utf-8" ?>
<result>
<row>
- <label>/this is cool!</label>
+ <label>/ééé"&#039;... <this is cool>!</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
--
GitLab