Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disables auto sanitizing for Annotation API #22552

Merged
merged 5 commits into from
Sep 6, 2024
Merged

Disables auto sanitizing for Annotation API #22552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
bb3682f
Improve type hinting for Annotation API and limit message to 255 chars
sgiehl Sep 3, 2024
0f13897
shorten message to 255 chars
sgiehl Sep 4, 2024
f5a8bdb
adjust tests
sgiehl Sep 4, 2024
9a929aa
updates expected UI test file
sgiehl Sep 4, 2024
0e94fbe
cs
sgiehl Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 80 additions & 51 deletions plugins/Annotations/API.php
View file
Open in desktop
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
Open in desktop
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
Loading