diff --git a/toolkit/components/places/nsNavBookmarks.cpp b/toolkit/components/places/nsNavBookmarks.cpp index 93732188f541..792aa0a6f75a 100644 --- a/toolkit/components/places/nsNavBookmarks.cpp +++ b/toolkit/components/places/nsNavBookmarks.cpp @@ -642,7 +642,7 @@ NS_IMETHODIMP nsNavBookmarks::RemoveItem(int64_t aItemId) { SAMPLE_LABEL("bookmarks", "RemoveItem"); - NS_ENSURE_ARG(aItemId != mRoot); + NS_ENSURE_ARG(!IsRoot(aItemId)); BookmarkData bookmark; nsresult rv = FetchItemInfo(aItemId, bookmark); @@ -1137,6 +1137,7 @@ nsNavBookmarks::RemoveFolderChildren(int64_t aFolderId) { SAMPLE_LABEL("bookmarks", "RemoveFolderChilder"); NS_ENSURE_ARG_MIN(aFolderId, 1); + NS_ENSURE_ARG(aFolderId != mRoot); BookmarkData folder; nsresult rv = FetchItemInfo(aFolderId, folder); @@ -1272,13 +1273,13 @@ nsNavBookmarks::RemoveFolderChildren(int64_t aFolderId) NS_IMETHODIMP nsNavBookmarks::MoveItem(int64_t aItemId, int64_t aNewParent, int32_t aIndex) { - NS_ENSURE_TRUE(aItemId != mRoot, NS_ERROR_INVALID_ARG); + NS_ENSURE_ARG(!IsRoot(aItemId)); NS_ENSURE_ARG_MIN(aItemId, 1); NS_ENSURE_ARG_MIN(aNewParent, 1); // -1 is append, but no other negative number is allowed. NS_ENSURE_ARG_MIN(aIndex, -1); // Disallow making an item its own parent. - NS_ENSURE_TRUE(aItemId != aNewParent, NS_ERROR_INVALID_ARG); + NS_ENSURE_ARG(aItemId != aNewParent); mozStorageTransaction transaction(mDB->MainConn(), false); diff --git a/toolkit/components/places/nsNavBookmarks.h b/toolkit/components/places/nsNavBookmarks.h index d145b13f5d11..0e3b7acdf2a8 100644 --- a/toolkit/components/places/nsNavBookmarks.h +++ b/toolkit/components/places/nsNavBookmarks.h @@ -283,6 +283,12 @@ private: int64_t mUnfiledRoot; int64_t mToolbarRoot; + inline bool IsRoot(int64_t aFolderId) { + return aFolderId == mRoot || aFolderId == mMenuRoot || + aFolderId == mTagsRoot || aFolderId == mUnfiledRoot || + aFolderId == mToolbarRoot; + } + nsresult IsBookmarkedInDatabase(int64_t aBookmarkID, bool* aIsBookmarked); nsresult SetItemDateInternal(enum mozilla::places::BookmarkDate aDateType, diff --git a/toolkit/components/places/nsNavHistoryQuery.cpp b/toolkit/components/places/nsNavHistoryQuery.cpp index 03d3b26e1c55..1b6f7bdd7add 100644 --- a/toolkit/components/places/nsNavHistoryQuery.cpp +++ b/toolkit/components/places/nsNavHistoryQuery.cpp @@ -213,43 +213,38 @@ namespace PlacesFolderConversion { * @param aFolderID * The folder ID to convert to the proper named constant. */ - inline void AppendFolder(nsCString &aQuery, int64_t aFolderID) + inline nsresult AppendFolder(nsCString &aQuery, int64_t aFolderID) { nsNavBookmarks *bs = nsNavBookmarks::GetBookmarksService(); + NS_ENSURE_STATE(bs); int64_t folderID; - (void)bs->GetPlacesRoot(&folderID); - if (aFolderID == folderID) { + if (NS_SUCCEEDED(bs->GetPlacesRoot(&folderID)) && + aFolderID == folderID) { aQuery.AppendLiteral(PLACES_ROOT_FOLDER); - return; } - - (void)bs->GetBookmarksMenuFolder(&folderID); - if (aFolderID == folderID) { + else if (NS_SUCCEEDED(bs->GetBookmarksMenuFolder(&folderID)) && + aFolderID == folderID) { aQuery.AppendLiteral(BOOKMARKS_MENU_FOLDER); - return; } - - (void)bs->GetTagsFolder(&folderID); - if (aFolderID == folderID) { + else if (NS_SUCCEEDED(bs->GetTagsFolder(&folderID)) && + aFolderID == folderID) { aQuery.AppendLiteral(TAGS_FOLDER); - return; } - - (void)bs->GetUnfiledBookmarksFolder(&folderID); - if (aFolderID == folderID) { + else if (NS_SUCCEEDED(bs->GetUnfiledBookmarksFolder(&folderID)) && + aFolderID == folderID) { aQuery.AppendLiteral(UNFILED_BOOKMARKS_FOLDER); - return; } - - (void)bs->GetToolbarFolder(&folderID); - if (aFolderID == folderID) { + else if (NS_SUCCEEDED(bs->GetToolbarFolder(&folderID)) && + aFolderID == folderID) { aQuery.AppendLiteral(TOOLBAR_FOLDER); - return; + } + else { + // It wasn't one of our named constants, so just convert it to a string. + aQuery.AppendInt(aFolderID); } - // It wasn't one of our named constants, so just convert it to a string - aQuery.AppendInt(aFolderID); + return NS_OK; } } @@ -473,7 +468,8 @@ nsNavHistory::QueriesToQueryString(nsINavHistoryQuery **aQueries, for (uint32_t i = 0; i < folderCount; ++i) { AppendAmpersandIfNonempty(queryString); queryString += NS_LITERAL_CSTRING(QUERYKEY_FOLDER "="); - PlacesFolderConversion::AppendFolder(queryString, folders[i]); + nsresult rv = PlacesFolderConversion::AppendFolder(queryString, folders[i]); + NS_ENSURE_SUCCESS(rv, rv); } nsMemory::Free(folders); diff --git a/toolkit/components/places/tests/bookmarks/test_protectRoots.js b/toolkit/components/places/tests/bookmarks/test_protectRoots.js new file mode 100644 index 000000000000..7822d4c564c3 --- /dev/null +++ b/toolkit/components/places/tests/bookmarks/test_protectRoots.js @@ -0,0 +1,36 @@ +/* Any copyright is dedicated to the Public Domain. + http://creativecommons.org/publicdomain/zero/1.0/ */ + +function run_test() +{ + const ROOTS = [ + PlacesUtils.bookmarksMenuFolderId, + PlacesUtils.toolbarFolderId, + PlacesUtils.unfiledBookmarksFolderId, + PlacesUtils.tagsFolderId, + PlacesUtils.placesRootId + ]; + + for (let root of ROOTS) { + do_check_true(PlacesUtils.isRootItem(root)); + + try { + PlacesUtils.bookmarks.removeItem(root); + do_throw("Trying to remove a root should throw"); + } catch (ex) {} + + try { + PlacesUtils.bookmarks.moveItem(root, PlacesUtils.placesRootId, 0); + do_throw("Trying to move a root should throw"); + } catch (ex) {} + + try { + PlacesUtils.bookmarks.removeFolderChildren(root); + if (root == PlacesUtils.placesRootId) + do_throw("Trying to remove children of the main root should throw"); + } catch (ex) { + if (root != PlacesUtils.placesRootId) + do_throw("Trying to remove children of other roots should not throw"); + } + } +} diff --git a/toolkit/components/places/tests/bookmarks/xpcshell.ini b/toolkit/components/places/tests/bookmarks/xpcshell.ini index e37d2f37b5a3..c0a263dbf736 100644 --- a/toolkit/components/places/tests/bookmarks/xpcshell.ini +++ b/toolkit/components/places/tests/bookmarks/xpcshell.ini @@ -30,3 +30,4 @@ tail = [test_savedsearches.js] [test_675416.js] [test_711914.js] +[test_protectRoots.js]