Prevent logging tracking failure for obviously malformed idsite or token auth

Author: sgiehl

core/Tracker/Request.php

@@ -206,7 +206,10 @@ protected function authenticateTrackingApi(
             if ($this->isAuthenticated) {
                 Common::printDebug("token_auth is authenticated!");
             } else {
-                StaticContainer::get('Piwik\Tracker\Failures')->logFailure(Failures::FAILURE_ID_NOT_AUTHENTICATED, $this);
+                if (preg_match('/^\w{28,36}$/', $tokenAuth) || empty($tokenAuth)) {
+                    // only log a failure if the token auth looks partial valid or is completely missing
+                    StaticContainer::get('Piwik\Tracker\Failures')->logFailure(Failures::FAILURE_ID_NOT_AUTHENTICATED, $this);
+                }
             }
         } else {
             $this->isAuthenticated = true;

core/Tracker/RequestHandlerTrait.php

@@ -31,10 +31,12 @@ protected function checkSiteExists(Request $request): void
         try {
             $request->getIdSite();
         } catch (UnexpectedWebsiteFoundException $e) {
-            // we allow 0... the request will fail anyway as the site won't exist... allowing 0 will help us
-            // reporting this tracking problem as it is a common issue. Otherwise we would not be able to report
-            // this problem in tracking failures
-            StaticContainer::get(Failures::class)->logFailure(Failures::FAILURE_ID_INVALID_SITE, $request);
+            $idSite = $this->request->getRawParams()['idsite'] ?? null;
+            if (is_numeric($idSite) && (string)(int)$idSite === (string)$idSite && (int)$idSite >= 0) {
+                // only log a failure in case the provided idsite was valid positive integer
+                StaticContainer::get(Failures::class)->logFailure(Failures::FAILURE_ID_INVALID_SITE, $request);
+            }
+
             throw $e;
         }
     }

tests/PHPUnit/Integration/Tracker/FailuresTest.php

@@ -10,11 +10,13 @@
 namespace Piwik\Tests\Integration\Tracker;
 
 use Piwik\Date;
+use Piwik\Exception\InvalidRequestParameterException;
 use Piwik\Exception\UnexpectedWebsiteFoundException;
 use Piwik\Tests\Framework\Fixture;
 use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
 use Piwik\Tracker\Failures;
 use Piwik\Tracker\Request;
+use Piwik\Tracker\Visit;
 
 /**
  * @group Failures
@@ -301,6 +303,112 @@ public function testRemoveFailuresOlderThanDays()
         ), $summary);
     }
 
+
+    /**
+     * @dataProvider getInvalidSiteIds
+     */
+    public function testProvidingInvalidSiteIdForTrackingDoesLogFailure(string $idsite)
+    {
+        try {
+            $request = new Request(['idsite' => $idsite, 'rec' => '1', 'url' => 'https://matomo.org/index']);
+            $visit   = new Visit();
+            $visit->setRequest($request);
+            $visit->handle();
+            self::fail('expected exception not raised');
+        } catch (UnexpectedWebsiteFoundException $e) {
+            // ignore, as we expect a UnexpectedWebsiteFoundException to be thrown
+        }
+
+        self::assertCount(1, $this->failures->getAllFailures());
+    }
+
+    public function getInvalidSiteIds(): array
+    {
+        return [
+            ['4'],
+            ['0'],
+            ['1234'],
+        ];
+    }
+
+    /**
+     * @dataProvider getMalFormedSiteIds
+     */
+    public function testProvidingMalformedSiteIdForTrackingDoesNotLogFailure(string $idsite)
+    {
+        try {
+            $request = new Request(['idsite' => $idsite, 'rec' => '1', 'url' => 'https://matomo.org/index']);
+            $visit   = new Visit();
+            $visit->setRequest($request);
+            $visit->handle();
+            self::fail('expected exception not raised');
+        } catch (UnexpectedWebsiteFoundException $e) {
+            // ignore, as we expect it to be thrown
+        }
+
+        self::assertCount(0, $this->failures->getAllFailures());
+    }
+
+    public function getMalFormedSiteIds(): array
+    {
+        return [
+            [''],
+            ['-4'],
+            ['1"; DROP TABLE'],
+            ['5,6'],
+            ['nan'],
+            ['test5'],
+        ];
+    }
+
+    public function testProvidingInvalidTokenAuthForTrackingDoesLogFailure()
+    {
+        try {
+            $request = new Request(['idsite' => '1', 'rec' => '1', 'url' => 'https://matomo.org/index', 'city' => 'Berlin'], '1d34ghdrg6j33uersadfg34defg342vs');
+            $visit   = new Visit();
+            $visit->setRequest($request);
+            $visit->handle();
+            self::fail('expected exception not raised');
+        } catch (InvalidRequestParameterException $e) {
+            // ignore, as we expect that exception
+        }
+
+        self::assertCount(1, $this->failures->getAllFailures());
+    }
+
+    /**
+     * @dataProvider getMalFormedTokenAuths
+     */
+    public function testProvidingMalformedTokenAuthForTrackingDoesNotLogFailure(string $tokenAuth)
+    {
+        try {
+            $request = new Request(['idsite' => '1', 'rec' => '1', 'url' => 'https://matomo.org/index', 'city' => 'Berlin'], $tokenAuth);
+            $visit   = new Visit();
+            $visit->setRequest($request);
+            $visit->handle();
+            self::fail('expected exception not raised');
+        } catch (InvalidRequestParameterException $e) {
+            // ignore, as we expect that exception
+        }
+
+        self::assertCount(0, $this->failures->getAllFailures());
+    }
+
+    public function getMalFormedTokenAuths(): iterable
+    {
+        yield 'too short token' => [
+            'abc',
+        ];
+
+        yield 'too long token' => [
+            'abc2435tgadetb356z2wq3er4gbrnz367634rahne735e5wqergwert245hw45h3gq45h',
+        ];
+
+        yield 'token with invalid chars' => [
+            '33dc3f!536d302$974cccb4b4d2-98f4',
+        ];
+    }
+
     private function getFailureSummary()
     {
         $failures = $this->failures->getAllFailures();