From f6ec24415260a0e28e1caf6b1f7049e9b31d2273 Mon Sep 17 00:00:00 2001 From: Marcel Klehr Date: Wed, 16 Aug 2023 20:19:22 +0200 Subject: [PATCH] enh(javascript,file): Allow creating javascript and file links fixes #832 fixes #1468 Signed-off-by: Marcel Klehr --- docs/bookmark.rst | 9 ++++- docs/changes.rst | 10 ++++++ lib/Controller/BookmarkController.php | 11 +++--- lib/Controller/InternalBookmarkController.php | 10 +++--- lib/Db/Bookmark.php | 16 +++++++++ lib/Db/BookmarkMapper.php | 8 +++-- lib/Db/BookmarkWithTagsAndParent.php | 9 +++++ lib/Db/TreeMapper.php | 8 ++--- lib/Service/BookmarkPreviewer.php | 4 +++ lib/Service/BookmarkService.php | 27 +++++++++----- lib/Service/CrawlService.php | 3 ++ lib/Service/FaviconPreviewer.php | 3 ++ lib/Service/LinkExplorer.php | 3 +- src/components/BookmarkContent.vue | 5 ++- src/components/Item.vue | 8 +++-- src/components/SidebarBookmark.vue | 36 ++++++++++--------- 16 files changed, 123 insertions(+), 47 deletions(-) diff --git a/docs/bookmark.rst b/docs/bookmark.rst index 1155ddcf..5911b0dc 100644 --- a/docs/bookmark.rst +++ b/docs/bookmark.rst @@ -14,7 +14,14 @@ A bookmark has at least the following properties .. object:: Bookmark :param int id: The bookmarks unique id - :param string url: The Uniform Resource Locator that this bookmark represents + :param string url: The Uniform Resource Locator that this bookmark represents, can be a http, ftp or (since v13.1.0) a file link + + .. versionchanged:: 13.1.0 + + :param string target: The target of this bookmark, can be a http, ftp or file link or a javascript link + + .. versionadded:: 13.1.0 + :param string title: A short humanly readable label for the bookmark :param string description: A longer description or note on the bookmark :param int added: The UNIX timestamp when this bookmark was created diff --git a/docs/changes.rst b/docs/changes.rst index 28afdf23..9496dbc1 100644 --- a/docs/changes.rst +++ b/docs/changes.rst @@ -5,6 +5,16 @@ Changes .. _changes: +Changes in v13.1.0 +================== + +In v13.1.0 the ``target`` field for bookmarks was introduced, which largely replaces the ``url`` field. +The ``url`` field still exists and can be used, however, while the ``url`` field is guaranteed to only contain http(s), (s)ftp and file links, +the ``target`` field may also contain javascript links. If a bookmark represents a javascript link, the ``url`` field will be set to the empty string. +The ``target`` field was introduced as a security consideration taking into account downstream implementors of this API that +may expose the contents of the ``url`` field directly as links that users click on, and which do not expect this field to contain javascript. + + Breaking changes from v2.x to v3.x ================================== diff --git a/lib/Controller/BookmarkController.php b/lib/Controller/BookmarkController.php index 615c78ee..d2dd9a84 100644 --- a/lib/Controller/BookmarkController.php +++ b/lib/Controller/BookmarkController.php @@ -404,6 +404,7 @@ class BookmarkController extends ApiController { * @param string $description * @param array $tags * @param array $folders + * @param string $target * @return JSONResponse * * @NoAdminRequired @@ -411,7 +412,7 @@ class BookmarkController extends ApiController { * @CORS * @PublicPage */ - public function newBookmark($url = '', $title = null, $description = null, $tags = null, $folders = []): JSONResponse { + public function newBookmark($url = '', $title = null, $description = null, $tags = null, $folders = [], $target = null): JSONResponse { $permissions = Authorizer::PERM_ALL; $this->authorizer->setCredentials($this->request); foreach ($folders as $folder) { @@ -425,7 +426,7 @@ class BookmarkController extends ApiController { $folders = array_map(function ($folderId) { return $this->toInternalFolderId($folderId); }, $folders); - $bookmark = $this->bookmarks->create($this->authorizer->getUserId(), $url, $title, $description, $tags, $folders); + $bookmark = $this->bookmarks->create($this->authorizer->getUserId(), $target ?: $url, $title, $description, $tags, $folders); return new JSONResponse(['item' => $this->_returnBookmarkAsArray($bookmark), 'status' => 'success']); } catch (AlreadyExistsError $e) { // This is really unlikely, as we make sure to use the existing one if it already exists @@ -451,6 +452,7 @@ class BookmarkController extends ApiController { * @param string|null $description * @param array|null $tags * @param array|null $folders + * @param string|null $target * @return JSONResponse * * @NoAdminRequired @@ -458,7 +460,7 @@ class BookmarkController extends ApiController { * @CORS * @PublicPage */ - public function editBookmark($id = null, $url = null, $title = null, $description = null, $tags = null, $folders = null): JSONResponse { + public function editBookmark($id = null, $url = null, $title = null, $description = null, $tags = null, $folders = null, $target = null): JSONResponse { if (!Authorizer::hasPermission(Authorizer::PERM_EDIT, $this->authorizer->getPermissionsForBookmark($id, $this->request))) { return new JSONResponse(['status' => 'error', 'data' => 'Unauthorized'], Http::STATUS_FORBIDDEN); } @@ -476,12 +478,13 @@ class BookmarkController extends ApiController { return new JSONResponse(['status' => 'error', 'data' => ['Insufficient permissions']], Http::STATUS_FORBIDDEN); } } - $bookmark = $this->bookmarks->update($this->authorizer->getUserId(), $id, $url, $title, $description, $tags, $folders); + $bookmark = $this->bookmarks->update($this->authorizer->getUserId(), $id, $target ?: $url, $title, $description, $tags, $folders); return new JSONResponse(['item' => $bookmark ? $this->_returnBookmarkAsArray($bookmark) : null, 'status' => 'success']); } catch (AlreadyExistsError $e) { // This is really unlikely, as we make sure to use the existing one if it already exists return new JSONResponse(['status' => 'error', 'data' => 'Bookmark already exists'], Http::STATUS_BAD_REQUEST); } catch (UrlParseError $e) { + $this->logger->error($e->getMessage(), ['exception' => $e]); return new JSONResponse(['status' => 'error', 'data' => 'Invald URL'], Http::STATUS_BAD_REQUEST); } catch (UserLimitExceededError $e) { return new JSONResponse(['status' => 'error', 'data' => 'User limit exceeded'], Http::STATUS_BAD_REQUEST); diff --git a/lib/Controller/InternalBookmarkController.php b/lib/Controller/InternalBookmarkController.php index 703ea35c..57abdbd3 100644 --- a/lib/Controller/InternalBookmarkController.php +++ b/lib/Controller/InternalBookmarkController.php @@ -95,12 +95,13 @@ class InternalBookmarkController extends ApiController { * @param string $description * @param array $tags * @param array $folders + * @param string $target * @return JSONResponse * * @NoAdminRequired */ - public function newBookmark($url = "", $title = null, $description = null, $tags = null, $folders = []): JSONResponse { - return $this->publicController->newBookmark($url, $title, $description, $tags, $folders); + public function newBookmark($url = "", $title = null, $description = null, $tags = null, $folders = [], $target = null): JSONResponse { + return $this->publicController->newBookmark($url, $title, $description, $tags, $folders, $target); } /** @@ -110,12 +111,13 @@ class InternalBookmarkController extends ApiController { * @param string $description * @param array $tags * @param array|null $folders + * @param string|null $target * @return JSONResponse * * @NoAdminRequired */ - public function editBookmark($id = null, $url = null, $title = null, $description = "", $tags = [], $folders = null): JSONResponse { - return $this->publicController->editBookmark($id, $url, $title, $description, $tags, $folders); + public function editBookmark($id = null, $url = null, $title = null, $description = "", $tags = [], $folders = null, $target = null): JSONResponse { + return $this->publicController->editBookmark($id, $url, $title, $description, $tags, $folders, $target); } /** diff --git a/lib/Db/Bookmark.php b/lib/Db/Bookmark.php index db73ca88..baa433ba 100644 --- a/lib/Db/Bookmark.php +++ b/lib/Db/Bookmark.php @@ -57,6 +57,9 @@ class Bookmark extends Entity { public static function fromArray($props): self { $bookmark = new Bookmark(); foreach ($props as $prop => $val) { + if ($prop === 'target') { + $prop = 'url'; + } $bookmark->{'set' . $prop}($val); } return $bookmark; @@ -80,6 +83,15 @@ class Bookmark extends Entity { public function toArray(): array { $array = []; foreach (self::$fields as $field) { + if ($field === 'url') { + if (!preg_match('/^javascript:/i', $this->url)) { + $array['url'] = $this->url; + } else { + $array['url'] = ''; + } + $array['target'] = $this->url; + continue; + } $array[$field] = $this->{$field}; } return $array; @@ -108,4 +120,8 @@ class Bookmark extends Entity { } $this->setter('description', [$desc]); } + + public function isWebLink() { + return (bool) preg_match('/^https?:/i', $this->getUrl()); + } } diff --git a/lib/Db/BookmarkMapper.php b/lib/Db/BookmarkMapper.php index d7dc2f1d..c5e43d53 100644 --- a/lib/Db/BookmarkMapper.php +++ b/lib/Db/BookmarkMapper.php @@ -691,7 +691,9 @@ class BookmarkMapper extends QBMapper { */ public function update(Entity $entity): Bookmark { // normalize url - $entity->setUrl($this->urlNormalizer->normalize($entity->getUrl())); + if ($entity->isWebLink()) { + $entity->setUrl($this->urlNormalizer->normalize($entity->getUrl())); + } $entity->setLastmodified(time()); return parent::update($entity); } @@ -711,7 +713,9 @@ class BookmarkMapper extends QBMapper { } // normalize url - $entity->setUrl($this->urlNormalizer->normalize($entity->getUrl())); + if ($entity->isWebLink()) { + $entity->setUrl($this->urlNormalizer->normalize($entity->getUrl())); + } if ($entity->getAdded() === null) { $entity->setAdded(time()); diff --git a/lib/Db/BookmarkWithTagsAndParent.php b/lib/Db/BookmarkWithTagsAndParent.php index 3177f466..36cca71c 100644 --- a/lib/Db/BookmarkWithTagsAndParent.php +++ b/lib/Db/BookmarkWithTagsAndParent.php @@ -37,6 +37,15 @@ class BookmarkWithTagsAndParent extends Bookmark { } continue; } + if ($field === 'url') { + if (!preg_match('/^javascript:/i', $this->url)) { + $array['url'] = $this->url; + } else { + $array['url'] = ''; + } + $array['target'] = $this->url; + continue; + } $array[$field] = $this->{$field}; } return $array; diff --git a/lib/Db/TreeMapper.php b/lib/Db/TreeMapper.php index 0e4f5372..5988efcd 100644 --- a/lib/Db/TreeMapper.php +++ b/lib/Db/TreeMapper.php @@ -824,9 +824,7 @@ class TreeMapper extends QBMapper { $indices = array_column($children, 'index'); array_multisort($indices, $children); - $bookmark = new Bookmark(); - - $children = array_map(function ($child) use ($layers, $bookmark) { + $children = array_map(function ($child) use ($layers) { $item = ['type' => $child['type'], 'id' => (int)$child['id'], 'title' => $child['title'], 'userId' => $child['user_id']]; if ($item['type'] === self::TYPE_SHARE) { @@ -835,9 +833,7 @@ class TreeMapper extends QBMapper { } if ($item['type'] === self::TYPE_BOOKMARK) { - foreach (Bookmark::$columns as $col) { - $item[$bookmark->columnToProperty($col)] = $child[$col]; - } + $item = array_merge($item, Bookmark::fromRow($child)->toArray()); } if ($item['type'] === self::TYPE_FOLDER && $layers !== 0) { diff --git a/lib/Service/BookmarkPreviewer.php b/lib/Service/BookmarkPreviewer.php index 41d3d535..e34e2733 100644 --- a/lib/Service/BookmarkPreviewer.php +++ b/lib/Service/BookmarkPreviewer.php @@ -80,6 +80,10 @@ class BookmarkPreviewer implements IBookmarkPreviewer { return null; } + if (!$bookmark->isWebLink()) { + return null; + } + $previewers = [$this->screeenlyPreviewer, $this->screenshotMachinePreviewer, $this->pageresPreviewer, $this->defaultPreviewer]; foreach ($previewers as $previewer) { $key = $previewer::CACHE_PREFIX . '-' . md5($bookmark->getUrl()); diff --git a/lib/Service/BookmarkService.php b/lib/Service/BookmarkService.php index 50a9ba6f..0aff52da 100644 --- a/lib/Service/BookmarkService.php +++ b/lib/Service/BookmarkService.php @@ -28,6 +28,7 @@ use OCP\BackgroundJob\IJobList; use OCP\EventDispatcher\IEventDispatcher; class BookmarkService { + public const PROTOCOLS_REGEX = '/^(https?|s?ftp|file|javascript):/i'; /** * @var BookmarkMapper */ @@ -175,18 +176,18 @@ class BookmarkService { */ private function _addBookmark($userId, $url, string $title = null, $description = null, array $tags = null, array $folders = []): Bookmark { $bookmark = null; + try { $bookmark = $this->bookmarkMapper->findByUrl($userId, $url); } catch (DoesNotExistException $e) { - $protocols = '/^(https?|s?ftp):\/\//i'; - if (!preg_match($protocols, $url)) { + if (!preg_match(self::PROTOCOLS_REGEX, $url)) { // if no allowed protocol is given, evaluate https and https foreach (['https://', 'http://'] as $protocol) { - $testUrl = $this->urlNormalizer->normalize($protocol . $url); try { + $testUrl = $this->urlNormalizer->normalize($protocol . $url); $bookmark = $this->bookmarkMapper->findByUrl($userId, $testUrl); break; - } catch (DoesNotExistException $e) { + } catch (UrlParseError|DoesNotExistException $e) { continue; } } @@ -199,21 +200,26 @@ class BookmarkService { if (!isset($title, $description)) { // Inspect web page (do some light scraping) // allow only http(s) and (s)ftp - $protocols = '/^(https?|s?ftp)\:\/\//i'; - if (preg_match($protocols, $url)) { - $data = $this->linkExplorer->get($url); + if (preg_match('/^https?:\/\//i', $url)) { + $testUrl = $this->urlNormalizer->normalize($url); + $data = $this->linkExplorer->get($testUrl); } else { // if no allowed protocol is given, evaluate https and https - foreach (['https://', 'http://'] as $protocol) { + foreach (['https://', 'http://', ''] as $protocol) { $testUrl = $protocol . $url; $data = $this->linkExplorer->get($testUrl); if (isset($data['basic']['title'])) { + $url = $protocol . $url; break; } } } } + if (!preg_match(self::PROTOCOLS_REGEX, $url)) { + throw new UrlParseError(); + } + $url = $data['url'] ?? $url; $title = $title ?? $data['basic']['title'] ?? $url; $title = trim($title); @@ -279,6 +285,9 @@ class BookmarkService { } if ($url !== null) { + if (!preg_match(self::PROTOCOLS_REGEX, $url)) { + throw new UrlParseError(); + } if ($url !== $bookmark->getUrl()) { $bookmark->setAvailable(true); } @@ -322,6 +331,8 @@ class BookmarkService { * @var $currentOwnFolders Folder[] */ $currentOwnFolders = $this->treeMapper->findParentsOf(TreeMapper::TYPE_BOOKMARK, $bookmark->getId()); + // Updating user may not be the owner of the bookmark + // We have to keep the bookmark in folders that are inaccessible to the current user if ($bookmark->getUserId() !== $userId) { $currentInaccessibleOwnFolders = array_map(static function ($f) { return $f->getId(); diff --git a/lib/Service/CrawlService.php b/lib/Service/CrawlService.php index 5866db51..7f61a72e 100644 --- a/lib/Service/CrawlService.php +++ b/lib/Service/CrawlService.php @@ -82,6 +82,9 @@ class CrawlService { * @throws UrlParseError */ public function crawl(Bookmark $bookmark): void { + if (!$bookmark->isWebLink()) { + return; + } try { $client = new Client(); /** @var Response $resp */ diff --git a/lib/Service/FaviconPreviewer.php b/lib/Service/FaviconPreviewer.php index 448ba19a..9acc3718 100644 --- a/lib/Service/FaviconPreviewer.php +++ b/lib/Service/FaviconPreviewer.php @@ -72,6 +72,9 @@ class FaviconPreviewer implements IBookmarkPreviewer { if (!isset($bookmark)) { return null; } + if (!$bookmark->isWebLink()) { + return null; + } $key = self::CACHE_PREFIX . '-' . md5($bookmark->getUrl()); // Try cache first try { diff --git a/lib/Service/LinkExplorer.php b/lib/Service/LinkExplorer.php index 5f6fbdca..e449703c 100644 --- a/lib/Service/LinkExplorer.php +++ b/lib/Service/LinkExplorer.php @@ -7,7 +7,6 @@ namespace OCA\Bookmarks\Service; -use Exception; use Marcelklehr\LinkPreview\Client as LinkPreview; use OCA\Bookmarks\Http\Client; use OCA\Bookmarks\Http\RequestFactory; @@ -51,7 +50,7 @@ class LinkExplorer { try { libxml_use_internal_errors(false); $preview = $this->linkPreview->getLink($url)->getPreview(); - } catch (Exception $e) { + } catch (\Throwable $e) { $this->logger->debug($e->getMessage(), ['app' => 'bookmarks']); return $data; } diff --git a/src/components/BookmarkContent.vue b/src/components/BookmarkContent.vue index aad8caf1..48c39edb 100644 --- a/src/components/BookmarkContent.vue +++ b/src/components/BookmarkContent.vue @@ -5,7 +5,7 @@ -->
-
+
- {{ bookmark.url }} + {{ bookmark.target }} - + @@ -51,16 +51,16 @@
- + - + - + @@ -150,8 +150,8 @@ export default { return { title: '', editingTitle: false, - url: '', - editingUrl: false, + target: '', + editingTarget: false, activeTab: '', showContentModal: false, } @@ -216,6 +216,8 @@ export default { methods: { onClose() { this.$store.commit(mutations.SET_SIDEBAR, null) + this.editingTitle = false + this.editingTarget = false }, onNotesChange(e) { this.scheduleSave() @@ -244,18 +246,18 @@ export default { this.editingTitle = false this.title = '' }, - onEditUrl() { - this.url = this.bookmark.url - this.editingUrl = true + onEditTarget() { + this.target = this.bookmark.target + this.editingTarget = true }, - onEditUrlSubmit() { - this.editingUrl = false - this.bookmark.url = this.url + onEditTargetSubmit() { + this.editingTarget = false + this.bookmark.target = this.target this.scheduleSave() }, - onEditUrlCancel() { - this.editingUrl = false - this.url = '' + onEditTargetCancel() { + this.editingTarget = false + this.target = '' }, scheduleSave() { if (this.changeTimeout) clearTimeout(this.changeTimeout)