Skip to content

Commit

Permalink
Disables auto sanitizing for Annotation API (#22552)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@sgiehl
sgiehl authored Sep 6, 2024
1 parent 50477f7 commit 330596d
Show file tree
Hide file tree
Showing 6 changed files with 99 additions and 69 deletions.
131 changes: 80 additions & 51 deletions plugins/Annotations/API.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);

Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);

Expand All @@ -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.
Expand All @@ -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);
Expand All @@ -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(
Expand All @@ -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);
}
Expand All @@ -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
Expand All @@ -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;
}
Expand All @@ -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];
Expand All @@ -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;
Expand All @@ -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'));
Expand All @@ -296,30 +309,30 @@ 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.");
}
}

/**
* 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
Expand All @@ -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);
}
}
21 changes: 11 additions & 10 deletions plugins/Annotations/AnnotationList.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand All @@ -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);

Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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());
}

Expand Down Expand Up @@ -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'.");
}
}
Expand Down
Loading

0 comments on commit 330596d

Please sign in to comment.