From 330596ddf0901c8f0b7b28e79d0aa26e7e23bfe3 Mon Sep 17 00:00:00 2001 From: Stefan Giehl Date: Fri, 6 Sep 2024 12:04:59 +0200 Subject: [PATCH] Disables auto sanitizing for Annotation API (#22552) * Improve type hinting for Annotation API and limit message to 255 chars * shorten message to 255 chars * adjust tests * updates expected UI test file * cs --- plugins/Annotations/API.php | 131 +++++++++++------- plugins/Annotations/AnnotationList.php | 21 +-- .../Annotations/templates/_annotation.twig | 2 +- .../templates/_annotationList.twig | 2 +- .../tests/System/AnnotationsTest.php | 8 +- .../UIIntegrationTest_api_listing.png | 4 +- 6 files changed, 99 insertions(+), 69 deletions(-) diff --git a/plugins/Annotations/API.php b/plugins/Annotations/API.php index b6049e53b58..61d05b0c88e 100644 --- a/plugins/Annotations/API.php +++ b/plugins/Annotations/API.php @@ -10,8 +10,10 @@ namespace Piwik\Plugins\Annotations; use Exception; +use Piwik\Common; use Piwik\Date; use Piwik\Piwik; +use Piwik\Site; /** * @see plugins/Annotations/AnnotationList.php @@ -26,23 +28,28 @@ */ class API extends \Piwik\Plugin\API { + // do not automatically apply `Common::sanitizeInputValue` to all API parameters + protected $autoSanitizeInputParams = false; + /** * Create a new annotation for a site. * - * @param string $idSite The site ID to add the annotation to. + * @param int $idSite The site ID to add the annotation to. * @param string $date The date the annotation is attached to. - * @param string $note The text of the annotation. - * @param int $starred Either 0 or 1. Whether the annotation should be starred. + * @param string $note The text of the annotation (max 255 chars). + * @param boolean $starred Whether the annotation should be starred. * @return array Returns an array of two elements. The first element (indexed by * 'annotation') is the new annotation. The second element (indexed * by 'idNote' is the new note's ID). */ - public function add($idSite, $date, $note, $starred = 0) + public function add(int $idSite, string $date, string $note, bool $starred = false): array { $this->checkUserCanAddNotesFor($idSite); - $this->checkSingleIdSite($idSite, $extraMessage = "Note: Cannot add one note to multiple sites."); + $this->checkSiteExists($idSite); $this->checkDateIsValid($date); + $note = $this->filterNote($note); + // add, save & return a new annotation $annotations = new AnnotationList($idSite); @@ -62,28 +69,30 @@ public function add($idSite, $date, $note, $starred = 0) * - the user has view access, is not the anonymous user and is the user that * created the note * - * @param string $idSite The site ID to add the annotation to. - * @param string $idNote The ID of the note. + * @param int $idSite The site ID to add the annotation to. + * @param int $idNote The ID of the note. * @param string|null $date The date the annotation is attached to. If null, the annotation's * date is not modified. - * @param string|null $note The text of the annotation. If null, the annotation's text - * is not modified. - * @param string|null $starred Either 0 or 1. Whether the annotation should be starred. - * If null, the annotation is not starred/un-starred. + * @param string|null $note The text of the annotation (max 255 chars). + * If null, the annotation's text is not modified. + * @param bool|null $starred Whether the annotation should be starred. + * If null, the annotation is not starred/un-starred, so the current state won't change. * @return array Returns an array of two elements. The first element (indexed by * 'annotation') is the new annotation. The second element (indexed * by 'idNote' is the new note's ID). */ - public function save($idSite, $idNote, $date = null, $note = null, $starred = null) + public function save(int $idSite, int $idNote, ?string $date = null, ?string $note = null, ?bool $starred = null): array { - $this->checkSingleIdSite($idSite, $extraMessage = "Note: Cannot modify more than one note at a time."); + $this->checkSiteExists($idSite); $this->checkDateIsValid($date, $canBeNull = true); // get the annotations for the site $annotations = new AnnotationList($idSite); // check permissions - $this->checkUserCanModifyOrDelete($idSite, $annotations->get($idSite, $idNote)); + $this->checkUserCanModifyOrDelete($annotations->get($idSite, $idNote)); + + $note = $this->filterNote($note); // modify the annotation, and save the whole list $annotations->update($idSite, $idNote, $date, $note, $starred); @@ -101,17 +110,17 @@ public function save($idSite, $idNote, $date = null, $note = null, $starred = nu * - the user has view access, is not the anonymous user and is the user that * created the note * - * @param string $idSite The site ID to add the annotation to. - * @param string $idNote The ID of the note to delete. + * @param int $idSite The site ID to add the annotation to. + * @param int $idNote The ID of the note to delete. */ - public function delete($idSite, $idNote) + public function delete(int $idSite, int $idNote): void { - $this->checkSingleIdSite($idSite, $extraMessage = "Note: Cannot delete annotations from multiple sites."); + $this->checkSiteExists($idSite); $annotations = new AnnotationList($idSite); // check permissions - $this->checkUserCanModifyOrDelete($idSite, $annotations->get($idSite, $idNote)); + $this->checkUserCanModifyOrDelete($annotations->get($idSite, $idNote)); // remove the note & save the list $annotations->remove($idSite, $idNote); @@ -121,13 +130,13 @@ public function delete($idSite, $idNote) /** * Removes all annotations for a single site. Only super users can use this method. * - * @param string $idSite The ID of the site to remove annotations for. + * @param int $idSite The ID of the site to remove annotations for. */ - public function deleteAll($idSite) + public function deleteAll(int $idSite): void { Piwik::checkUserHasSuperUserAccess(); - $this->checkSingleIdSite($idSite, $extraMessage = "Note: Cannot delete annotations from multiple sites."); + $this->checkSiteExists($idSite); $annotations = new AnnotationList($idSite); @@ -139,8 +148,8 @@ public function deleteAll($idSite) /** * Returns a single note for one site. * - * @param string $idSite The site ID to add the annotation to. - * @param string $idNote The ID of the note to get. + * @param int $idSite The site ID to add the annotation to. + * @param int $idNote The ID of the note to get. * @return array The annotation. It will contain the following properties: * - date: The date the annotation was recorded for. * - note: The note text. @@ -149,11 +158,11 @@ public function deleteAll($idSite) * - canEditOrDelete: Whether the user that called this method can edit or * delete the annotation returned. */ - public function get($idSite, $idNote) + public function get(int $idSite, int $idNote): array { Piwik::checkUserHasViewAccess($idSite); - $this->checkSingleIdSite($idSite, $extraMessage = "Note: Specify only one site ID when getting ONE note."); + $this->checkSiteExists($idSite); // get single annotation $annotations = new AnnotationList($idSite); @@ -165,11 +174,11 @@ public function get($idSite, $idNote) * The date range is specified by a date, the period type (day/week/month/year) * and an optional number of N periods in the past to include. * - * @param string $idSite The site ID to add the annotation to. Can be one ID or + * @param string $idSite The site ID to get annotations for. Can be one ID or * a list of site IDs. - * @param bool|string $date The date of the period. + * @param null|string $date The date of the period. * @param string $period The period type. - * @param bool|int $lastN Whether to include the last N number of periods in the + * @param null|int $lastN Whether to include the last N number of periods in the * date range or not. * @return array An array that indexes arrays of annotations by site ID. ie, * array( @@ -180,14 +189,14 @@ public function get($idSite, $idNote) * 8 => array(...) * ) */ - public function getAll($idSite, $date = false, $period = 'day', $lastN = false) + public function getAll(string $idSite, ?string $date = null, string $period = 'day', ?int $lastN = null): array { Piwik::checkUserHasViewAccess($idSite); $annotations = new AnnotationList($idSite); // if date/period are supplied, determine start/end date for search - list($startDate, $endDate) = Annotations::getDateRangeForPeriod($date, $period, $lastN); + list($startDate, $endDate) = Annotations::getDateRangeForPeriod($date ?? false, $period, $lastN ?? false); return $annotations->search($startDate, $endDate); } @@ -196,8 +205,8 @@ public function getAll($idSite, $date = false, $period = 'day', $lastN = false) * Returns the count of annotations for a list of periods, including the count of * starred annotations. * - * @param string $idSite The site ID to add the annotation to. - * @param string|bool $date The date of the period. + * @param string $idSite The site ID(s) to get the annotation count for. + * @param string $date The date of the period. * @param string $period The period type. * @param int|bool $lastN Whether to get counts for the last N number of periods or not. * @param bool $getAnnotationText @@ -217,18 +226,23 @@ public function getAll($idSite, $date = false, $period = 'day', $lastN = false) * ... * ) */ - public function getAnnotationCountForDates($idSite, $date, $period, $lastN = false, $getAnnotationText = false) - { + public function getAnnotationCountForDates( + string $idSite, + string $date, + string $period, + ?int $lastN = null, + bool $getAnnotationText = false + ): array { Piwik::checkUserHasViewAccess($idSite); // get start & end date for request. lastN is ignored if $period == 'range' - list($startDate, $endDate) = Annotations::getDateRangeForPeriod($date, $period, $lastN); + list($startDate, $endDate) = Annotations::getDateRangeForPeriod($date, $period, $lastN ?? false); if ($period == 'range') { $period = 'day'; } // create list of dates - $dates = array(); + $dates = []; for (; $startDate->getTimestamp() <= $endDate->getTimestamp(); $startDate = $startDate->addPeriod(1, $period)) { $dates[] = $startDate; } @@ -239,7 +253,7 @@ public function getAnnotationCountForDates($idSite, $date, $period, $lastN = fal $annotations = new AnnotationList($idSite); // create result w/ 0-counts - $result = array(); + $result = []; for ($i = 0; $i != count($dates) - 1; ++$i) { $date = $dates[$i]; $nextDate = $dates[$i + 1]; @@ -266,10 +280,10 @@ public function getAnnotationCountForDates($idSite, $date, $period, $lastN = fal } // convert associative array into array of pairs (so it can be traversed by index) - $pairResult = array(); + $pairResult = []; foreach ($result as $idSite => $counts) { foreach ($counts as $date => $count) { - $pairResult[$idSite][] = array($date, $count); + $pairResult[$idSite][] = [$date, $count]; } } return $pairResult; @@ -278,11 +292,10 @@ public function getAnnotationCountForDates($idSite, $date, $period, $lastN = fal /** * Throws if the current user is not allowed to modify or delete an annotation. * - * @param int $idSite The site ID the annotation belongs to. * @param array $annotation The annotation. * @throws Exception if the current user is not allowed to modify/delete $annotation. */ - private function checkUserCanModifyOrDelete($idSite, $annotation) + private function checkUserCanModifyOrDelete($annotation): void { if (!$annotation['canEditOrDelete']) { throw new Exception(Piwik::translate('Annotations_YouCannotModifyThisNote')); @@ -296,7 +309,7 @@ private function checkUserCanModifyOrDelete($idSite, $annotation) * @throws Exception if the current user is anonymous or does not have view access * for site w/ id=$idSite. */ - private static function checkUserCanAddNotesFor($idSite) + private static function checkUserCanAddNotesFor($idSite): void { if (!AnnotationList::canUserAddNotesFor($idSite)) { throw new Exception("The current user is not allowed to add notes for site #$idSite."); @@ -304,22 +317,22 @@ private static function checkUserCanAddNotesFor($idSite) } /** - * Utility function, makes sure idSite string has only one site ID and throws if - * otherwise. + * Throws an exception if the given $idSite does not exist. + * + * @param int $idSite + * @return void + * @throws \Piwik\Exception\UnexpectedWebsiteFoundException */ - private function checkSingleIdSite($idSite, $extraMessage) + private function checkSiteExists(int $idSite): void { - // can only add a note to one site - if (!is_numeric($idSite)) { - throw new Exception("Invalid idSite: '$idSite'. $extraMessage"); - } + new Site($idSite); } /** * Utility function, makes sure date string is valid date, and throws if * otherwise. */ - private function checkDateIsValid($date, $canBeNull = false) + private function checkDateIsValid($date, $canBeNull = false): void { if ( $date === null @@ -330,4 +343,20 @@ private function checkDateIsValid($date, $canBeNull = false) Date::factory($date); } + + private function filterNote(?string $note): ?string + { + if (empty($note)) { + return $note; + } + + // shorten note if longer than 255 characters + if (mb_strlen($note) > 255) { + $note = mb_substr($note, 0, 254) . '…'; + } + + // @todo store message unsanitized, sanitize on output instead. + // can be changed when migrating annotations to a separate table. + return Common::sanitizeInputValue($note); + } } diff --git a/plugins/Annotations/AnnotationList.php b/plugins/Annotations/AnnotationList.php index b48b6b3a49a..9a69c29876b 100644 --- a/plugins/Annotations/AnnotationList.php +++ b/plugins/Annotations/AnnotationList.php @@ -78,12 +78,12 @@ public function getIdSites() * @param int $idSite The ID of the site to add an annotation to. * @param string $date The date the annotation is in reference to. * @param string $note The text of the new annotation. - * @param int $starred Either 1 or 0. If 1, the new annotation has been starred, + * @param bool $starred If true the new annotation has been starred, * otherwise it will start out unstarred. * @return array The added annotation. * @throws Exception if $idSite is not an ID that was supplied upon construction. */ - public function add($idSite, $date, $note, $starred = 0) + public function add(int $idSite, string $date, string $note, bool $starred = false): array { $this->checkIdSiteIsLoaded($idSite); $date = Date::factory($date)->toString('Y-m-d'); @@ -103,7 +103,7 @@ public function add($idSite, $date, $note, $starred = 0) * @param int $idSite The ID of the site to save annotations for. * @throws Exception if $idSite is not an ID that was supplied upon construction. */ - public function save($idSite) + public function save($idSite): void { $this->checkIdSiteIsLoaded($idSite); @@ -128,7 +128,7 @@ public function save($idSite) * @throws Exception if $idSite is not an ID that was supplied upon construction. * @throws Exception if $idNote does not refer to valid note for the site. */ - public function update($idSite, $idNote, $date = null, $note = null, $starred = null) + public function update($idSite, $idNote, $date = null, $note = null, $starred = null): void { $this->checkIdSiteIsLoaded($idSite); $this->checkNoteExists($idSite, $idNote); @@ -193,10 +193,11 @@ public function removeAll($idSite) * * @param int $idSite The ID of the site to get an annotation for. * @param int $idNote The ID of the note to get. + * @return array * @throws Exception if $idSite is not an ID that was supplied upon construction. * @throws Exception if $idNote does not refer to valid note for the site. */ - public function get($idSite, $idNote) + public function get($idSite, $idNote): array { $this->checkIdSiteIsLoaded($idSite); $this->checkNoteExists($idSite, $idNote); @@ -308,14 +309,14 @@ public function count($idSite, $startDate, $endDate) * * @param string $date * @param string $note - * @param int $starred + * @param bool $starred * @return array */ - private function makeAnnotation($date, $note, $starred = 0) + private function makeAnnotation(string $date, string $note, bool $starred = false) { return array('date' => $date, 'note' => $note, - 'starred' => (int)$starred, + 'starred' => (int) $starred, 'user' => Piwik::getCurrentUserLogin()); } @@ -370,9 +371,9 @@ private function checkIdSiteIsLoaded($idSite) * @param int $idNote * @throws Exception */ - private function checkNoteExists($idSite, $idNote) + private function checkNoteExists($idSite, $idNote): void { - if (empty($this->annotations[$idSite][$idNote])) { + if (empty($this->annotations[$idSite][$idNote]) || !is_array($this->annotations[$idSite][$idNote])) { throw new Exception("There is no note with id '$idNote' for site with id '$idSite'."); } } diff --git a/plugins/Annotations/templates/_annotation.twig b/plugins/Annotations/templates/_annotation.twig index 7776e2de98a..4c351169c55 100644 --- a/plugins/Annotations/templates/_annotation.twig +++ b/plugins/Annotations/templates/_annotation.twig @@ -36,7 +36,7 @@ {% if annotation.canEditOrDelete %}