Skip to content

Commit 74ba3aa

Browse files
sgiehlsgiehl
authored
Avoid possible problems with invalid segments (matomo-org#21574)
* Avoid possible fatal error getSqlSegment() on null * Ensure to pass idSite when using segment in DataComparisonFilter * Use new Request class in DataComparisonFilter * Adjust tests to avoid regressions * fix another usage of segment without idsite
1 parent abf5aa7 commit 74ba3aa

3 files changed

Lines changed: 48 additions & 24 deletions

File tree

‎core/Segment.php‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,6 +404,11 @@ protected function getCleanedExpression($expression)
404404
}
405405

406406
$segmentObject = $segmentsList->getSegment($name);
407+
408+
if (empty($segmentObject)) {
409+
throw new Exception("Segment '$name' is not a supported segment.");
410+
}
411+
407412
$sqlName = $segmentObject->getSqlSegment();
408413

409414
$joinTable = null;

‎plugins/API/Filter/DataComparisonFilter.php‎

Lines changed: 26 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ class DataComparisonFilter
131131

132132
public function __construct($request, Report $report = null)
133133
{
134-
$this->request = $request;
134+
$this->request = new \Piwik\Request($request);
135135

136136
$generalConfig = Config::getInstance()->General;
137137
$this->segmentCompareLimit = (int) $generalConfig['data_comparison_segment_limit'];
@@ -173,7 +173,7 @@ public function __construct($request, Report $report = null)
173173
$this->comparePeriodIndices[$period][$date] = $index;
174174
}
175175

176-
$this->invertCompareChangeCompute = Common::getRequestVar('invert_compare_change_compute', $default = 0, $type = 'int', $request) == 1;
176+
$this->invertCompareChangeCompute = $this->request->getIntegerParameter('invert_compare_change_compute', 0) === 1;
177177
if ($this->invertCompareChangeCompute && count($this->comparePeriods) != 2) {
178178
throw new \Exception("invert_compare_change_compute=1 can only be used when comparing two periods.");
179179
}
@@ -200,8 +200,8 @@ public function compare(DataTable\DataTableInterface $table)
200200
return;
201201
}
202202

203-
$method = Common::getRequestVar('method', $default = null, $type = 'string', $this->request);
204-
if ($method == 'Live') {
203+
$method = $this->request->getStringParameter('method');
204+
if ($method === 'Live') {
205205
throw new \Exception("Data comparison is not enabled for the Live API.");
206206
}
207207

@@ -295,31 +295,31 @@ private function requestReport($method, $paramsToModify)
295295
'disable_queued_filters' => 1,
296296
'format_metrics' => 0,
297297
'label' => '',
298-
'flat' => Common::getRequestVar('flat', 0, 'int', $this->request),
299-
'filter_add_columns_when_show_all_columns' => Common::getRequestVar('filter_add_columns_when_show_all_columns', '', 'string', $this->request),
300-
'filter_update_columns_when_show_all_goals' => Common::getRequestVar('filter_update_columns_when_show_all_goals', '', 'string', $this->request),
301-
'filter_show_goal_columns_process_goals' => Common::getRequestVar('filter_show_goal_columns_process_goals', '', 'string', $this->request),
302-
'idGoal' => Common::getRequestVar('idGoal', '', 'string', $this->request),
298+
'flat' => $this->request->getIntegerParameter('flat', 0),
299+
'filter_add_columns_when_show_all_columns' => $this->request->getStringParameter('filter_add_columns_when_show_all_columns', ''),
300+
'filter_update_columns_when_show_all_goals' => $this->request->getStringParameter('filter_update_columns_when_show_all_goals', ''),
301+
'filter_show_goal_columns_process_goals' => $this->request->getStringParameter('filter_show_goal_columns_process_goals', ''),
302+
'idGoal' => $this->request->getStringParameter('idGoal', ''),
303303
],
304304
$paramsToModify
305305
);
306306

307-
$params['keep_totals_row'] = Common::getRequestVar('keep_totals_row', 0, 'int', $this->request);
308-
$params['keep_totals_row_label'] = Common::getRequestVar('keep_totals_row_label', '', 'string', $this->request);
307+
$params['keep_totals_row'] = $this->request->getIntegerParameter('keep_totals_row', 0);
308+
$params['keep_totals_row_label'] = $this->request->getStringParameter('keep_totals_row_label', '');
309309

310310
if (!isset($params['idSite'])) {
311-
$params['idSite'] = Common::getRequestVar('idSite', null, 'string', $this->request);
311+
$params['idSite'] = $this->request->getStringParameter('idSite');
312312
}
313313
if (!isset($params['period'])) {
314-
$params['period'] = Common::getRequestVar('period', null, 'string', $this->request);
314+
$params['period'] = $this->request->getStringParameter('period');
315315
}
316316
if (!isset($params['date'])) {
317-
$params['date'] = Common::getRequestVar('date', null, 'string', $this->request);
317+
$params['date'] = $this->request->getStringParameter('date');
318318
}
319319

320-
$idSubtable = Common::getRequestVar('idSubtable', 0, 'int', $this->request);
320+
$idSubtable = $this->request->getIntegerParameter('idSubtable', 0);
321321
if ($idSubtable > 0) {
322-
$comparisonIdSubtables = Common::getRequestVar('comparisonIdSubtables', $default = false, 'json', $this->request);
322+
$comparisonIdSubtables = $this->request->getJsonParameter('comparisonIdSubtables', false);
323323
if (empty($comparisonIdSubtables)) {
324324
throw new \Exception("Comparing segments/periods with subtables only works when the comparison idSubtables are supplied as well.");
325325
}
@@ -389,8 +389,10 @@ private function getMetadataFromModifiedParams($modifiedParams)
389389

390390
$metadata['compareSegment'] = $segment;
391391

392-
$segmentObj = new Segment($segment, []);
393-
$metadata['compareSegmentPretty'] = $segmentObj->getStoredSegmentName(false);
392+
$idSite = $modifiedParams['idSite'] ?? $this->request->getStringParameter('idSite');
393+
394+
$segmentObj = new Segment($segment, [$idSite]);
395+
$metadata['compareSegmentPretty'] = $segmentObj->getStoredSegmentName($idSite);
394396

395397
$metadata['comparePeriod'] = $period;
396398
$metadata['compareDate'] = $date;
@@ -457,8 +459,8 @@ private function checkMultiplePeriodCompare()
457459
private function isRequestMultiplePeriod()
458460
{
459461
if ($this->isRequestMultiplePeriod === null) {
460-
$period = Common::getRequestVar('period', $default = null, 'string', $this->request);
461-
$date = Common::getRequestVar('date', $default = null, 'string', $this->request);
462+
$period = $this->request->getStringParameter('period');
463+
$date = $this->request->getStringParameter('date');
462464

463465
$this->isRequestMultiplePeriod = Period::isMultiplePeriod($date, $period);
464466
}
@@ -599,7 +601,7 @@ private static function getCompareDates($request = null)
599601
*/
600602
private function shouldIncludeTrendValues(): bool
601603
{
602-
return (bool) Common::getRequestVar('include_trends', 0, 'int', $this->request);
604+
return $this->request->getBoolParameter('include_trends', false);
603605
}
604606

605607
/**
@@ -615,8 +617,9 @@ public static function getPrettyComparisonLabelFromSeriesIndex($labelSeriesIndex
615617

616618
[$periodIndex, $segmentIndex] = self::getIndividualComparisonRowIndices(null, $labelSeriesIndex, count($compareSegments));
617619

618-
$segmentObj = new Segment($compareSegments[$segmentIndex], []);
619-
$prettySegment = $segmentObj->getStoredSegmentName(false);
620+
$idSite = \Piwik\Request::fromRequest()->getStringParameter('idSite');
621+
$segmentObj = new Segment($compareSegments[$segmentIndex], [$idSite]);
622+
$prettySegment = $segmentObj->getStoredSegmentName($idSite);
620623

621624
$prettyPeriod = Factory::build($comparePeriods[$periodIndex], $compareDates[$periodIndex])->getLocalizedLongString();
622625
$prettyPeriod = ucfirst($prettyPeriod);

‎tests/PHPUnit/System/DataComparisonTest.php‎

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010

1111
use Piwik\API\Request;
1212
use Piwik\DataTable;
13+
use Piwik\Plugins\CustomDimensions\CustomDimensions;
1314
use Piwik\Tests\Fixtures\ManySitesImportedLogs;
15+
use Piwik\Tests\Framework\Fixture;
1416
use Piwik\Tests\Framework\TestCase\SystemTestCase;
1517
/**
1618
* Testing Data comparison
@@ -24,6 +26,20 @@ class DataComparisonTest extends SystemTestCase
2426
*/
2527
public static $fixture;
2628

29+
public static function setUpBeforeClass(): void
30+
{
31+
parent::setUpBeforeClass();
32+
33+
// Configures a custom dimensions and adds a segment for it. This segment will only be available for
34+
// the specific site and will be invalid in global context
35+
// Added to avoid further regressions like: https://github.com/matomo-org/matomo/issues/21573
36+
$idDimension = \Piwik\Plugins\CustomDimensions\API::getInstance()->configureNewCustomDimension(
37+
self::$fixture->idSite, 'test', CustomDimensions::SCOPE_VISIT, true
38+
);
39+
Fixture::clearInMemoryCaches(false);
40+
\Piwik\Plugins\SegmentEditor\API::getInstance()->add('custom dimension', "dimension$idDimension==test", self::$fixture->idSite);
41+
}
42+
2743
/**
2844
* @dataProvider getApiForTesting
2945
*/
@@ -48,7 +64,7 @@ public function testSubtableComparisons()
4864
['Referrers.getWebsites', 'Referrers.getUrlsFromWebsiteId'],
4965
];
5066

51-
foreach ($apiToTest as list($superApiMethod, $subtableApiMethod)) {
67+
foreach ($apiToTest as [$superApiMethod, $subtableApiMethod]) {
5268
/** @var DataTable $topLevelComparisons */
5369
$topLevelComparisons = Request::processRequest($superApiMethod, [
5470
'idSite' => self::$fixture->idSite,

0 commit comments

Comments
 (0)