From 482cf02b00876f799516036cef52c061136a0954 Mon Sep 17 00:00:00 2001 From: dizzy Date: Fri, 18 Jun 2021 00:14:35 -0700 Subject: fix regression in trackingspamprevention and warning in bound parameter checking code (#17683) * remove testdox * fix warning if parameters is a single value, not an array * allow skipping valid host check for hardcoded URLs we know are valid * print testdox hint in case build takes too long to finish consistently * move testdox warning to correct boostrap file * fixing some tests + fix use of Date in bind params --- config/global.php | 6 ++-- core/DataAccess/Model.php | 4 +-- core/Db.php | 6 +++- core/Http.php | 41 ++++++++++++++-------- plugins/TrackingSpamPrevention | 2 +- .../TwoFactorAuth/tests/UI/TwoFactorAuth_spec.js | 2 ++ tests/PHPUnit/bootstrap.php | 6 ++++ tests/PHPUnit/phpunit.xml.dist | 2 +- tests/javascript/index.php | 3 ++ 9 files changed, 49 insertions(+), 23 deletions(-) diff --git a/config/global.php b/config/global.php index 14fb7b50aa..4be44c4dbd 100644 --- a/config/global.php +++ b/config/global.php @@ -179,9 +179,9 @@ return array( * This defines a list of hostnames Matomo's Http class will deny requests to. Wildcards (*) can be used in the * beginning to match any subdomain level or in the end to match any tlds */ - 'http.blocklist.hosts' => DI\add([ - '*.amazonaws.com' - ]), + 'http.blocklist.hosts' => [ + '*.amazonaws.com', + ], 'Piwik\Tracker\VisitorRecognizer' => DI\autowire() ->constructorParameter('trustCookiesOnly', DI\get('ini.Tracker.trust_visitors_cookies')) diff --git a/core/DataAccess/Model.php b/core/DataAccess/Model.php index 40528f60bd..a15cbf337a 100644 --- a/core/DataAccess/Model.php +++ b/core/DataAccess/Model.php @@ -327,8 +327,8 @@ class Model /** @var Period $period */ $dateConditions[] = "(date1 <= ? AND ? <= date2)"; - $bind[] = $period->getDateStart(); - $bind[] = $period->getDateEnd(); + $bind[] = $period->getDateStart()->getDatetime(); + $bind[] = $period->getDateEnd()->getDatetime(); $dateConditionsSql = implode(" OR ", $dateConditions); $periodConditions[] = "(period = 5 AND ($dateConditionsSql))"; diff --git a/core/Db.php b/core/Db.php index 1fd14f439b..09e99b32ce 100644 --- a/core/Db.php +++ b/core/Db.php @@ -817,12 +817,16 @@ class Db Log::debug("Db::%s() executing SQL: %s", $functionName, $sql); } - private static function checkBoundParametersIfInDevMode($sql, $parameters = []) + private static function checkBoundParametersIfInDevMode($sql, $parameters) { if (!Development::isEnabled()) { return; } + if (!is_array($parameters)) { + $parameters = [$parameters]; + } + foreach ($parameters as $index => $parameter) { if ($parameter instanceof Date) { throw new \Exception("Found bound parameter (index = $index) is Date instance which will not work correctly in following SQL: $sql"); diff --git a/core/Http.php b/core/Http.php index 70ead69693..500cfe8e46 100644 --- a/core/Http.php +++ b/core/Http.php @@ -68,6 +68,8 @@ class Http * @param string $httpMethod The HTTP method to use. Defaults to `'GET'`. * @param string $httpUsername HTTP Auth username * @param string $httpPassword HTTP Auth password + * @param bool $checkHostIsAllowed whether we should check if the target host is allowed or not. This should only + * be set to false when using a hardcoded URL. * * @throws Exception if the response cannot be saved to `$destinationPath`, if the HTTP response cannot be sent, * if there are more than 5 redirects or if the request times out. @@ -93,13 +95,16 @@ class Http $getExtendedInfo = false, $httpMethod = 'GET', $httpUsername = null, - $httpPassword = null) + $httpPassword = null, + $checkHostIsAllowed = true) { // create output file $file = self::ensureDestinationDirectoryExists($destinationPath); $acceptLanguage = $acceptLanguage ? 'Accept-Language: ' . $acceptLanguage : ''; - return self::sendHttpRequestBy(self::getTransportMethod(), $aUrl, $timeout, $userAgent, $destinationPath, $file, $followDepth, $acceptLanguage, $acceptInvalidSslCertificate = false, $byteRange, $getExtendedInfo, $httpMethod, $httpUsername, $httpPassword); + return self::sendHttpRequestBy(self::getTransportMethod(), $aUrl, $timeout, $userAgent, $destinationPath, $file, + $followDepth, $acceptLanguage, $acceptInvalidSslCertificate = false, $byteRange, $getExtendedInfo, $httpMethod, + $httpUsername, $httpPassword, null, [], null, $checkHostIsAllowed); } public static function ensureDestinationDirectoryExists($destinationPath) @@ -160,6 +165,8 @@ class Http * @param string $httpPassword HTTP Auth password * @param array|string $requestBody If $httpMethod is 'POST' this may accept an array of variables or a string that needs to be posted * @param array $additionalHeaders List of additional headers to set for the request + * @param bool $checkHostIsAllowed whether we should check if the target host is allowed or not. This should only + * be set to false when using a hardcoded URL. * * @return string|array true (or string/array) on success; false on HTTP response error code (1xx or 4xx) *@throws Exception @@ -181,7 +188,8 @@ class Http $httpPassword = null, $requestBody = null, $additionalHeaders = array(), - $forcePost = null + $forcePost = null, + $checkHostIsAllowed = true ) { if ($followDepth > 5) { throw new Exception('Too many redirects (' . $followDepth . ')'); @@ -212,21 +220,24 @@ class Http )); } - $disallowedHosts = StaticContainer::get('http.blocklist.hosts'); - $isBlocked = false; + if ($checkHostIsAllowed) { + $disallowedHosts = StaticContainer::get('http.blocklist.hosts'); - foreach ($disallowedHosts as $host) { - if (preg_match(self::convertWildcardToPattern($host), $parsedUrl['host']) === 1) { - $isBlocked = true; - break; + $isBlocked = false; + + foreach ($disallowedHosts as $host) { + if (preg_match(self::convertWildcardToPattern($host), $parsedUrl['host']) === 1) { + $isBlocked = true; + break; + } } - } - if ($isBlocked) { - throw new Exception(sprintf( - 'Hostname %s is in list of disallowed hosts', - $parsedUrl['host'] - )); + if ($isBlocked) { + throw new Exception(sprintf( + 'Hostname %s is in list of disallowed hosts', + $parsedUrl['host'] + )); + } } $contentLength = 0; diff --git a/plugins/TrackingSpamPrevention b/plugins/TrackingSpamPrevention index 80153a1e4f..6068d61940 160000 --- a/plugins/TrackingSpamPrevention +++ b/plugins/TrackingSpamPrevention @@ -1 +1 @@ -Subproject commit 80153a1e4f0b280ab2ed73f56bdf173d7438abf0 +Subproject commit 6068d619400f80c92d2a30062425b33e31909d71 diff --git a/plugins/TwoFactorAuth/tests/UI/TwoFactorAuth_spec.js b/plugins/TwoFactorAuth/tests/UI/TwoFactorAuth_spec.js index 7d87184e19..26e97fd4d1 100644 --- a/plugins/TwoFactorAuth/tests/UI/TwoFactorAuth_spec.js +++ b/plugins/TwoFactorAuth/tests/UI/TwoFactorAuth_spec.js @@ -45,6 +45,8 @@ describe("TwoFactorAuth", function () { } await page.waitFor(1000); await page.goto(logMeUrl); + await page.waitForNetworkIdle(); + await page.waitFor(1000); } function requireTwoFa() { diff --git a/tests/PHPUnit/bootstrap.php b/tests/PHPUnit/bootstrap.php index 88e5362054..f74696e954 100644 --- a/tests/PHPUnit/bootstrap.php +++ b/tests/PHPUnit/bootstrap.php @@ -135,6 +135,7 @@ $config = Config::getInstance(); prepareServerVariables($config); prepareTestDatabaseConfig($config); checkPiwikSetupForTests(); +printTestDoxHint(); function checkPiwikSetupForTests() { @@ -156,3 +157,8 @@ Try again."; } } + +function printTestDoxHint() +{ + print "\nIf these tests time out consistently, it can be helpful to temporarily set testdox=true in the phpunit.xml.dist in order to see which test is causing the issue.\n"; +} \ No newline at end of file diff --git a/tests/PHPUnit/phpunit.xml.dist b/tests/PHPUnit/phpunit.xml.dist index 52a4a46997..0af257f09b 100644 --- a/tests/PHPUnit/phpunit.xml.dist +++ b/tests/PHPUnit/phpunit.xml.dist @@ -15,7 +15,7 @@ stopOnFailure="false" stopOnIncomplete="false" stopOnSkipped="false" - testdox="true" + testdox="false" verbose="true"> diff --git a/tests/javascript/index.php b/tests/javascript/index.php index ddec6a81e7..098809656f 100644 --- a/tests/javascript/index.php +++ b/tests/javascript/index.php @@ -22,7 +22,9 @@ if (!empty($_GET['plugin']) } try { + @ob_start(); $mysql = include_once $root . "/tests/PHPUnit/bootstrap.php"; + @ob_end_clean(); } catch (Exception $e) { echo 'alert("ERROR, not all tests are running! --> ' . $e->getMessage() . '")'; $mysql = false; @@ -1012,6 +1014,7 @@ function PiwikTest() { strictEqual(actual, _e('firstLink'), "findFirstNodeHavingAttributeWithValue, should find first link within body"); actual = query.findFirstNodeHavingAttributeWithValue(document.body, 'src'); + strictEqual(actual, _e('image2'), "findFirstNodeHavingAttributeWithValue, should not return first image which has empty src attribute"); -- cgit v1.2.3