enh(javascript,file): Allow creating javascript and file links

fixes #832
fixes #1468

Signed-off-by: Marcel Klehr <mklehr@gmx.net>

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

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
 ==================================
 

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);

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);
 	}
 
 	/**

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());
+	}
 }

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());

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;

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) {

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());

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();

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 */

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 {

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;
 		}

src/components/BookmarkContent.vue

@@ -5,7 +5,7 @@
   -->
 
 <template>
-	<div v-if="isActive && hasMinLength && !archivedFile" class="bookmark-content">
+	<div v-if="isActive && hasMinLength && !archivedFile && isWebLink" class="bookmark-content">
 		<div v-if="bookmark.textContent" class="content" v-html="content" />
 		<div v-else>
 			<NcEmptyContent :title="t('bookmarks', 'Content pending')"
@@ -34,6 +34,9 @@ export default {
 			if (!this.$store.state.sidebar) return false
 			return this.$store.state.sidebar.type === 'bookmark'
 		},
+		isWebLink() {
+			return this.bookmark.url.startsWith('http')
+		},
 		bookmark() {
 			if (!this.isActive) return
 			return this.$store.getters.getBookmark(this.$store.state.sidebar.id)

src/components/Item.vue

@@ -15,7 +15,8 @@
 		:draggable="draggable && !renaming"
 		@dragstart="onDragStart">
 		<template v-if="!renaming">
-			<a :href="url"
+			<component :is="url? 'a' : 'span'"
+				:href="url"
 				class="item__clickLink"
 				tabindex="0"
 				target="_blank"
@@ -43,7 +44,7 @@
 						<slot name="actions" />
 					</NcActions>
 				</div>
-			</a>
+			</component>
 		</template>
 		<div v-else class="item__rename">
 			<slot name="icon" />
@@ -179,6 +180,9 @@ export default {
 				e.stopImmediatePropagation()
 				return
 			}
+			if (!this.url) {
+				return
+			}
 			this.$emit('click', e)
 		},
 		onRightClick(e) {

src/components/SidebarBookmark.vue

@@ -38,11 +38,11 @@
 				<InformationVariantIcon />
 			</template>
 			<div>
-				<div v-if="!editingUrl" class="details__line">
+				<div v-if="!editingTarget" class="details__line">
 					<OpenInNewIcon :aria-label="t('bookmarks', 'Link')" :title="t('bookmarks', 'Link')" />
-					<a class="details__url" :href="bookmark.url">{{ bookmark.url }}</a>
+					<a class="details__url" :href="bookmark.target === bookmark.url ? bookmark.target : 'javascript:void(0)'">{{ bookmark.target }}</a>
 					<NcActions v-if="isEditable" class="details__action">
-						<NcActionButton @click="onEditUrl">
+						<NcActionButton @click="onEditTarget">
 							<template #icon>
 								<PencilIcon />
 							</template>
@@ -51,16 +51,16 @@
 				</div>
 				<div v-else class="details__line">
 					<OpenInNewIcon :aria-label="t('bookmarks', 'Link')" :title="t('bookmarks', 'Link')" />
-					<input v-model="url" class="details__url">
+					<input v-model="target" class="details__url">
 					<NcActions class="details__action">
-						<NcActionButton @click="onEditUrlSubmit">
+						<NcActionButton @click="onEditTargetSubmit">
 							<template #icon>
 								<ArrowRightIcon />
 							</template>
 						</NcActionButton>
 					</NcActions>
 					<NcActions class="details__action">
-						<NcActionButton @click="onEditUrlCancel">
+						<NcActionButton @click="onEditTargetCancel">
 							<template #icon>
 								<CloseIcon />
 							</template>
@@ -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)