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

9 files changed, 48 insertions, 22 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
-Subproject 80153a1e4f0b280ab2ed73f56bdf173d7438abf
+Subproject 6068d619400f80c92d2a30062425b33e31909d7
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">
 
 <testsuites>
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");