properly encode segment definitions from table so the hash will be the same as from query params (#17029)

* initial fix

* when creating a Segment or getting a segment hash using a segment definition from the segment table, make sure to urlencode so the hash is the same as when we send the segment as a query parameter

* try to make segment definition change appear in tests (seems like this is just a test fix? if I undo the changes the new test changes still pass)

* add test demonstrating bug

* check access in exampleplugin

* Add missing use

* fix test

* better fix

* fix tests

* fix test

* update expected screenshots

4 files changed, 10 insertions, 9 deletions

diff --git a/core/CronArchive.php b/core/CronArchive.php
index 8d634000ba..cc70f52062 100644
--- a/core/CronArchive.php
+++ b/core/CronArchive.php
@@ -885,11 +885,11 @@ class CronArchive
             }
 
             foreach ($this->segmentArchiving->getAllSegmentsToArchive($idSite) as $segmentDefinition) {
-                $params = new Parameters(new Site($idSite), $periodObj, new Segment($segmentDefinition, [$idSite], $periodObj->getDateStart(), $periodObj->getDateEnd()));
+                $params = new Parameters(new Site($idSite), $periodObj, new Segment(urlencode($segmentDefinition), [$idSite], $periodObj->getDateStart(), $periodObj->getDateEnd()));
                 if ($this->isThereExistingValidPeriod($params)) {
                     $this->logger->debug('  Found usable archive for {archive}, skipping invalidation.', ['archive' => $params]);
                 } else {
-                    $this->getApiToInvalidateArchivedReport()->invalidateArchivedReports($idSite, $date, $period, $segmentDefinition,
+                    $this->getApiToInvalidateArchivedReport()->invalidateArchivedReports($idSite, $date, $period, urlencode($segmentDefinition),
                         $cascadeDown = false, $_forceInvalidateNonexistant);
                 }
             }
diff --git a/core/CronArchive/QueueConsumer.php b/core/CronArchive/QueueConsumer.php
index 54b2be9ed1..ff751abd0d 100644
--- a/core/CronArchive/QueueConsumer.php
+++ b/core/CronArchive/QueueConsumer.php
@@ -510,6 +510,7 @@ class QueueConsumer
         $hash = substr($flag, 4);
         $storedSegment = $this->segmentArchiving->findSegmentForHash($hash, $archive['idsite']);
         if (!isset($storedSegment['definition'])) {
+            $this->logger->debug("Could not find stored segment for done flag hash: $flag");
             $archive['segment'] = null;
             return false;
         }
diff --git a/core/CronArchive/SegmentArchiving.php b/core/CronArchive/SegmentArchiving.php
index 923f3d4004..666e6b3d55 100644
--- a/core/CronArchive/SegmentArchiving.php
+++ b/core/CronArchive/SegmentArchiving.php
@@ -77,20 +77,20 @@ class SegmentArchiving
         $this->segmentListCache = $segmentListCache ?: new Transient();
         $this->now = $now ?: Date::factory('now');
         $this->logger = $logger ?: StaticContainer::get('Psr\Log\LoggerInterface');
-        $this->forceArchiveAllSegments = $this->getShouldForceArchiveAllSegments();
+        $this->forceArchiveAllSegments = self::getShouldForceArchiveAllSegments();
     }
 
     public function findSegmentForHash($hash, $idSite)
     {
         foreach ($this->getAllSegments() as $segment) {
             if (!$this->isAutoArchivingEnabledFor($segment)
-                || !$this->isSegmentForSite($segment, $idSite)
+                || !self::isSegmentForSite($segment, $idSite)
             ) {
                 continue;
             }
 
             try {
-                $segmentObj = new Segment($segment['definition'], [$idSite]);
+                $segmentObj = new Segment(urlencode($segment['definition']), [$idSite]);
             } catch (\Exception $ex) {
                 $this->logger->debug("Could not process segment {$segment['definition']} for site {$idSite}. Segment should not exist for the site, but does.");
                 continue;
@@ -205,7 +205,7 @@ class SegmentArchiving
         return Rules::getSegmentsToProcess([$idSite]);
     }
 
-    private function isSegmentForSite($segment, $idSite)
+    public static function isSegmentForSite($segment, $idSite)
     {
         return $segment['enable_only_idsite'] == 0
             || $segment['enable_only_idsite'] == $idSite;
@@ -216,7 +216,7 @@ class SegmentArchiving
         return $this->forceArchiveAllSegments || !empty($storedSegment['auto_archive']);
     }
 
-    private function getShouldForceArchiveAllSegments()
+    public static function getShouldForceArchiveAllSegments()
     {
         return !Rules::isBrowserTriggerEnabled() && !Rules::isBrowserArchivingAvailableForSegments();
     }
@@ -227,7 +227,7 @@ class SegmentArchiving
             return;
         }
 
-        $definition = $segmentInfo['definition'];
+        $definition = urlencode($segmentInfo['definition']);
         $idSite = $segmentInfo['enable_only_idsite'] ?? 'all';
 
         $idSites = Access::doAsSuperUser(function () use ($idSite) {
diff --git a/core/DataAccess/Model.php b/core/DataAccess/Model.php
index 82cec23c06..54fe187eb1 100644
--- a/core/DataAccess/Model.php
+++ b/core/DataAccess/Model.php
@@ -621,7 +621,7 @@ class Model
     private function getDeletedSegmentWhereClause(array $segment)
     {
         $idSite = (int)$segment['enable_only_idsite'];
-        $segmentHash = Segment::getSegmentHash($segment['definition']);
+        $segmentHash = Segment::getSegmentHash(urlencode($segment['definition']));
         // Valid segment hashes are md5 strings - just confirm that it is so it's safe for SQL injection
         if (!ctype_xdigit($segmentHash)) {
             throw new Exception($segment . ' expected to be an md5 hash');