From 09ca1f36c64bf36006240d619a3e4b6d30d1fb01 Mon Sep 17 00:00:00 2001 From: Abhishek Chandra Date: Sun, 6 Sep 2026 02:26:57 +0530 Subject: [PATCH 1/2] Guard bookmark folder traversal against cyclic folder relations A folder that ends up inside one of its own descendants makes every recursive walk over the relations table loop until the stack overflows: the folder picker (getFolderTree), deleting or fetching a folder branch (traverseBranch) and the HTML export (populateNode). Each walk now keeps the set of folders already on its path and skips any folder it has seen. Re-inserting an existing folder also replaced its parent relation instead of adding a second one, since Relation's primary key is autogenerated and REPLACE never deduplicated it. Fixes #5928 --- .../model/SavedSitesRepositoryTest.kt | 66 +++++++++++++++++++ .../service/SavedSitesExporterTest.kt | 25 +++++++ .../savedsites/impl/SavedSitesRepository.kt | 29 ++++++-- .../impl/service/SavedSitesExporter.kt | 12 ++-- 4 files changed, 121 insertions(+), 11 deletions(-) diff --git a/app/src/test/java/com/duckduckgo/app/bookmarks/model/SavedSitesRepositoryTest.kt b/app/src/test/java/com/duckduckgo/app/bookmarks/model/SavedSitesRepositoryTest.kt index c13b47b05eb3..de455a8a4385 100644 --- a/app/src/test/java/com/duckduckgo/app/bookmarks/model/SavedSitesRepositoryTest.kt +++ b/app/src/test/java/com/duckduckgo/app/bookmarks/model/SavedSitesRepositoryTest.kt @@ -769,6 +769,42 @@ class SavedSitesRepositoryTest { assertNull(repository.getBookmark(childBookmark.url)) } + @Test + fun whenFolderRelationsContainCycleThenGetFolderBranchTerminates() = runTest { + val parentFolder = + BookmarkFolder("folder1", "Parent Folder", SavedSitesNames.BOOKMARKS_ROOT, numBookmarks = 0, numFolders = 1, lastModified = "timestamp") + val childFolder = BookmarkFolder("folder2", "Child Folder", "folder1", numBookmarks = 1, numFolders = 0, lastModified = "timestamp") + val childBookmark = Bookmark("bookmark1", "title", "www.example.com", "folder2", "timestamp") + repository.insertFolderBranch(FolderBranch(listOf(childBookmark), listOf(parentFolder, childFolder))) + + // folder2 is already a child of folder1; making folder1 a child of folder2 closes the loop + savedSitesRelationsDao.insert(Relation(folderId = childFolder.id, entityId = parentFolder.id)) + + val branch = repository.getFolderBranch(parentFolder) + + assertTrue(branch.folders.any { it.id == parentFolder.id }) + assertTrue(branch.folders.any { it.id == childFolder.id }) + assertEquals(branch.folders.distinctBy { it.id }.size, branch.folders.size) + assertEquals(listOf(childBookmark), branch.bookmarks) + } + + @Test + fun whenFolderRelationsContainCycleThenDeleteFolderBranchTerminates() = runTest { + val parentFolder = + BookmarkFolder("folder1", "Parent Folder", SavedSitesNames.BOOKMARKS_ROOT, numBookmarks = 0, numFolders = 1, lastModified = "timestamp") + val childFolder = BookmarkFolder("folder2", "Child Folder", "folder1", numBookmarks = 1, numFolders = 0, lastModified = "timestamp") + val childBookmark = Bookmark("bookmark1", "title", "www.example.com", "folder2", "timestamp") + repository.insertFolderBranch(FolderBranch(listOf(childBookmark), listOf(parentFolder, childFolder))) + + savedSitesRelationsDao.insert(Relation(folderId = childFolder.id, entityId = parentFolder.id)) + + repository.deleteFolderBranch(parentFolder) + + assertNull(repository.getFolder(parentFolder.id)) + assertNull(repository.getFolder(childFolder.id)) + assertNull(repository.getBookmark(childBookmark.url)) + } + @Test fun whenBuildFlatStructureThenReturnFolderListWithDepth() = runTest { val rootFolder = BookmarkFolder(id = SavedSitesNames.BOOKMARKS_ROOT, name = "root", lastModified = "timestamp", parentId = "") @@ -799,6 +835,36 @@ class SavedSitesRepositoryTest { assertEquals(items, flatStructure) } + @Test + fun whenFolderRelationsContainCycleThenGetFolderTreeTerminates() = runTest { + val rootFolder = BookmarkFolder(id = SavedSitesNames.BOOKMARKS_ROOT, name = "root", lastModified = "timestamp", parentId = "") + val parentFolder = BookmarkFolder(id = "folder1", name = "name", lastModified = "timestamp", parentId = SavedSitesNames.BOOKMARKS_ROOT) + val childFolder = BookmarkFolder(id = "folder2", name = "another name", lastModified = "timestamp", parentId = "folder1") + + repository.insert(rootFolder) + repository.insert(parentFolder) + repository.insert(childFolder) + + // close the loop: folder1 becomes a child of folder2, which is already a child of folder1 + savedSitesRelationsDao.insert(Relation(folderId = childFolder.id, entityId = parentFolder.id)) + + val flatStructure = repository.getFolderTree(childFolder.id, null) + + assertEquals(flatStructure.distinctBy { it.bookmarkFolder.id }.size, flatStructure.size) + } + + @Test + fun whenSameFolderInsertedTwiceThenRelationIsNotDuplicated() = runTest { + val rootFolder = BookmarkFolder(id = SavedSitesNames.BOOKMARKS_ROOT, name = "root", lastModified = "timestamp", parentId = "") + val folder = BookmarkFolder(id = "folder1", name = "name", lastModified = "timestamp", parentId = SavedSitesNames.BOOKMARKS_ROOT) + + repository.insert(rootFolder) + repository.insert(folder) + repository.insert(folder) + + assertEquals(1, savedSitesRelationsDao.relationsByEntityId(folder.id).size) + } + @Test fun whenBuildFlatStructureThenReturnFolderListWithDepthWithoutCurrentFolderBranch() = runTest { val rootFolder = BookmarkFolder(id = SavedSitesNames.BOOKMARKS_ROOT, name = "root", lastModified = "timestamp", parentId = "") diff --git a/app/src/test/java/com/duckduckgo/app/bookmarks/service/SavedSitesExporterTest.kt b/app/src/test/java/com/duckduckgo/app/bookmarks/service/SavedSitesExporterTest.kt index b3887f1c32ab..35c0d90e3d96 100644 --- a/app/src/test/java/com/duckduckgo/app/bookmarks/service/SavedSitesExporterTest.kt +++ b/app/src/test/java/com/duckduckgo/app/bookmarks/service/SavedSitesExporterTest.kt @@ -38,10 +38,12 @@ import com.duckduckgo.savedsites.impl.RealFavoritesDelegate import com.duckduckgo.savedsites.impl.RealSavedSitesRepository import com.duckduckgo.savedsites.impl.service.RealSavedSitesExporter import com.duckduckgo.savedsites.impl.service.RealSavedSitesParser +import com.duckduckgo.savedsites.store.Relation import com.duckduckgo.savedsites.store.SavedSitesEntitiesDao import com.duckduckgo.savedsites.store.SavedSitesRelationsDao import kotlinx.coroutines.test.runTest import org.junit.* +import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.runner.RunWith import java.io.File @@ -189,6 +191,29 @@ class SavedSitesExporterTest { ) } + @Test + fun whenFolderRelationsContainCycleThenGetTreeStructureTerminates() = runTest { + val root = BookmarkFolder(SavedSitesNames.BOOKMARKS_ROOT, "DuckDuckGo Bookmarks", "", 0, 0, "timestamp") + val parentFolder = BookmarkFolder("folder1", "Folder One", SavedSitesNames.BOOKMARKS_ROOT, 0, 0, "timestamp") + val childFolder = BookmarkFolder("folder2", "Folder Two", "folder1", 0, 0, "timestamp") + val childBookmark = Bookmark("bookmark1", "title", "www.example.com", "folder2", "timestamp") + savedSitesRepository.insertFolderBranch(FolderBranch(listOf(childBookmark), listOf(root, parentFolder, childFolder))) + + // folder2 is already a child of folder1; making folder1 a child of folder2 closes the loop + savedSitesRelationsDao.insert(Relation(folderId = childFolder.id, entityId = parentFolder.id)) + + val treeStructure = exporter.getTreeFolderStructure() + + val visitedFolderIds = mutableListOf() + treeStructure.forEachVisit( + { node -> if (node.value.url == null) visitedFolderIds.add(node.value.id) }, + { }, + ) + + assertTrue(visitedFolderIds.containsAll(listOf(parentFolder.id, childFolder.id))) + assertEquals(visitedFolderIds.distinct().size, visitedFolderIds.size) + } + private fun testNode( node: TreeNode, itemList: List, diff --git a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/SavedSitesRepository.kt b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/SavedSitesRepository.kt index c66939e44d94..77b8ee4e7495 100644 --- a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/SavedSitesRepository.kt +++ b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/SavedSitesRepository.kt @@ -116,7 +116,15 @@ class RealSavedSitesRepository( return if (rootFolder != null) { val rootFolderItem = BookmarkFolderItem(0, rootFolder, rootFolder.id == selectedFolderId) val folders = mutableListOf(rootFolderItem) - val folderDepth = traverseFolderWithDepth(1, folders, SavedSitesNames.BOOKMARKS_ROOT, selectedFolderId = selectedFolderId, currentFolder) + val folderDepth = + traverseFolderWithDepth( + 1, + folders, + SavedSitesNames.BOOKMARKS_ROOT, + selectedFolderId = selectedFolderId, + currentFolder, + mutableSetOf(SavedSitesNames.BOOKMARKS_ROOT), + ) if (currentFolder != null) { folderDepth.filterNot { it.bookmarkFolder == currentFolder } } else { @@ -145,11 +153,13 @@ class RealSavedSitesRepository( folderId: String, selectedFolderId: String, currentFolder: BookmarkFolder?, + visitedFolderIds: MutableSet, ): List { getFolders(folderId).map { - if (it.id != currentFolder?.id) { + // folder relations can form a loop, so a folder already on this branch is never descended into again + if (it.id != currentFolder?.id && visitedFolderIds.add(it.id)) { folders.add(BookmarkFolderItem(depth, it, it.id == selectedFolderId)) - traverseFolderWithDepth(depth + 1, folders, it.id, selectedFolderId = selectedFolderId, currentFolder) + traverseFolderWithDepth(depth + 1, folders, it.id, selectedFolderId = selectedFolderId, currentFolder, visitedFolderIds) } } return folders @@ -169,7 +179,7 @@ class RealSavedSitesRepository( override fun getFolderBranch(folder: BookmarkFolder): FolderBranch { val bookmarks = mutableListOf() val folders = mutableListOf(folder) - val folderContent = traverseBranch(bookmarks, folders, folder.id) + val folderContent = traverseBranch(bookmarks, folders, folder.id, mutableSetOf(folder.id)) return FolderBranch(folderContent.first, folderContent.second) } @@ -177,12 +187,15 @@ class RealSavedSitesRepository( bookmarks: MutableList, folders: MutableList, folderId: String, + visitedFolderIds: MutableSet, ): Pair, List> { val folderContent = folderContent(folderId) bookmarks.addAll(folderContent.first) - folders.addAll(folderContent.second) - folderContent.second.forEach { - traverseBranch(bookmarks, folders, it.id) + // folder relations can form a loop, so a folder already on this branch is never descended into again + val unvisitedFolders = folderContent.second.filter { visitedFolderIds.add(it.id) } + folders.addAll(unvisitedFolders) + unvisitedFolders.forEach { + traverseBranch(bookmarks, folders, it.id, visitedFolderIds) } return Pair(bookmarks, folders) } @@ -434,6 +447,8 @@ class RealSavedSitesRepository( type = FOLDER, ) savedSitesEntitiesDao.insert(entity) + // a folder has a single parent, so re-inserting one has to replace its relation instead of adding a second + savedSitesRelationsDao.deleteRelationByEntity(folder.id) savedSitesRelationsDao.insert(Relation(folderId = folder.parentId, entityId = folder.id)) savedSitesEntitiesDao.updateModified(folder.parentId, folder.lastModified ?: DatabaseDateFormatter.iso8601()) return folder diff --git a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt index c06531b68c31..dccab0a20d2c 100644 --- a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt +++ b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/service/SavedSitesExporter.kt @@ -79,7 +79,7 @@ class RealSavedSitesExporter( @VisibleForTesting fun getTreeFolderStructure(): TreeNode { val node = TreeNode(FolderTreeItem(SavedSitesNames.BOOKMARKS_ROOT, RealSavedSitesParser.BOOKMARKS_FOLDER, "", null)) - populateNode(node, SavedSitesNames.BOOKMARKS_ROOT, 1) + populateNode(node, SavedSitesNames.BOOKMARKS_ROOT, 1, mutableSetOf(SavedSitesNames.BOOKMARKS_ROOT)) return node } @@ -87,13 +87,17 @@ class RealSavedSitesExporter( parentNode: TreeNode, parentId: String, currentDepth: Int, + visitedFolderIds: MutableSet, ) { val combinedContent = savedSitesRepository.getFolderTreeItems(parentId) combinedContent.forEach { item -> if (item.url == null) { - val childNode = TreeNode(item.copy(depth = currentDepth)) - parentNode.add(childNode) - populateNode(childNode, item.id, currentDepth + 1) + // folder relations can form a loop, so a folder already on this branch is never descended into again + if (visitedFolderIds.add(item.id)) { + val childNode = TreeNode(item.copy(depth = currentDepth)) + parentNode.add(childNode) + populateNode(childNode, item.id, currentDepth + 1, visitedFolderIds) + } } else { val childNode = TreeNode(item.copy(depth = currentDepth)) parentNode.add(childNode) From f34a04976a54002573b8cac9e4c93816317cffd3 Mon Sep 17 00:00:00 2001 From: Abhishek Chandra Date: Sun, 6 Sep 2026 03:23:50 +0530 Subject: [PATCH 2/2] Guard the sync folder walks against cyclic folder relations The sync data provider walks folder contents to build the upload and the sync repository walks up through parents after deduplication. Both recurse over the same relations table as the bookmark screens, so a loop between two folders overflowed the stack there as well. Each walk now skips a folder or entity it has already visited. --- .../model/SyncSavedSitesRepositoryTest.kt | 17 ++++++++++++++++ .../sync/SavedSitesSyncDataProviderTest.kt | 20 +++++++++++++++++++ .../impl/sync/RealSyncSavedSitesRepository.kt | 2 ++ .../impl/sync/SavedSitesSyncDataProvider.kt | 5 ++++- 4 files changed, 43 insertions(+), 1 deletion(-) diff --git a/app/src/test/java/com/duckduckgo/app/bookmarks/model/SyncSavedSitesRepositoryTest.kt b/app/src/test/java/com/duckduckgo/app/bookmarks/model/SyncSavedSitesRepositoryTest.kt index 4d66ef5b5aa1..ad54b8f3ad3e 100644 --- a/app/src/test/java/com/duckduckgo/app/bookmarks/model/SyncSavedSitesRepositoryTest.kt +++ b/app/src/test/java/com/duckduckgo/app/bookmarks/model/SyncSavedSitesRepositoryTest.kt @@ -647,6 +647,23 @@ class SyncSavedSitesRepositoryTest { assertTrue(savedSitesEntitiesDao.entityById(bookmarksRootFolder.id)!!.lastModified != oneHourAgo) } + @Test + fun whenFolderRelationsContainCycleThenSetLocalEntitiesForNextSyncTerminates() { + val twoHoursAgo = DatabaseDateFormatter.iso8601(OffsetDateTime.now(ZoneOffset.UTC).minusHours(2)) + val folderA = BookmarkFolder(id = "folderA", name = "Folder A", parentId = bookmarksRootFolder.id, lastModified = twoHoursAgo) + val folderB = BookmarkFolder(id = "folderB", name = "Folder B", parentId = folderA.id, lastModified = twoHoursAgo) + savedSitesRepository.insert(folderA) + savedSitesRepository.insert(folderB) + // folderB now also contains folderA, forming a loop in the relations table + savedSitesRelationsDao.insert(Relation(folderId = folderB.id, entityId = folderA.id)) + + val oneHourAhead = DatabaseDateFormatter.iso8601(OffsetDateTime.now(ZoneOffset.UTC).plusHours(1)) + repository.setLocalEntitiesForNextSync(oneHourAhead) + + assertTrue(savedSitesEntitiesDao.entityById(folderA.id) != null) + assertTrue(savedSitesEntitiesDao.entityById(folderB.id) != null) + } + @Test fun whenDeduplicatingBookmarkThenRemoteBookmarkReplacesLocal() { // given a local bookmark diff --git a/app/src/test/java/com/duckduckgo/app/sync/SavedSitesSyncDataProviderTest.kt b/app/src/test/java/com/duckduckgo/app/sync/SavedSitesSyncDataProviderTest.kt index 992c5e316677..34bc9479db41 100644 --- a/app/src/test/java/com/duckduckgo/app/sync/SavedSitesSyncDataProviderTest.kt +++ b/app/src/test/java/com/duckduckgo/app/sync/SavedSitesSyncDataProviderTest.kt @@ -51,6 +51,7 @@ import com.duckduckgo.savedsites.impl.sync.SyncSavedSitesRequestEntry import com.duckduckgo.savedsites.impl.sync.store.RealSavedSitesSyncEntitiesStore import com.duckduckgo.savedsites.impl.sync.store.SavedSitesSyncMetadataDao import com.duckduckgo.savedsites.impl.sync.store.SavedSitesSyncMetadataDatabase +import com.duckduckgo.savedsites.store.Relation import com.duckduckgo.savedsites.store.SavedSitesEntitiesDao import com.duckduckgo.savedsites.store.SavedSitesRelationsDao import com.duckduckgo.sync.api.SyncCrypto @@ -220,6 +221,25 @@ class SavedSitesSyncDataProviderTest { assertTrue(changes.bookmarks.updates[5].id == "bookmarks_root") } + @Test + fun whenFolderRelationsContainCycleThenGetChangesTerminatesAndListsEachFolderOnce() { + val folderA = aFolder("folderA", "Folder A", SavedSitesNames.BOOKMARKS_ROOT) + val folderB = aFolder("folderB", "Folder B", folderA.id) + repository.insert(folderA) + repository.insert(folderB) + repository.insert(bookmark3.copy(parentId = folderB.id)) + // folderB now also contains folderA, forming a loop in the relations table + savedSitesRelationsDao.insert(Relation(folderId = folderB.id, entityId = folderA.id)) + + val syncChanges = parser.getChanges() + + val changes = Adapters.adapter.fromJson(syncChanges.jsonString)!! + val updatedIds = changes.bookmarks.updates.map { it.id } + assertEquals(1, updatedIds.count { it == folderA.id }) + assertEquals(1, updatedIds.count { it == folderB.id }) + assertTrue(updatedIds.contains(bookmark3.id)) + } + @Test fun whenNewBookmarksSinceLastSyncThenChangesContainData() { repository.insert(bookmark3) diff --git a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/RealSyncSavedSitesRepository.kt b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/RealSyncSavedSitesRepository.kt index dece15e510c0..cb50c212d5c1 100644 --- a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/RealSyncSavedSitesRepository.kt +++ b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/RealSyncSavedSitesRepository.kt @@ -417,6 +417,8 @@ class RealSyncSavedSitesRepository( } private fun traverseParents(entity: String, entitiesToUpdate: MutableList) { + // folder relations can form a loop, so an entity already collected is never walked up from again + if (entitiesToUpdate.contains(entity)) return // find parent of each entity entitiesToUpdate.add(entity) val parents = savedSitesRelationsDao.relationsByEntityId(entity) diff --git a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/SavedSitesSyncDataProvider.kt b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/SavedSitesSyncDataProvider.kt index 2ab62604e6f7..893a4ec088c6 100644 --- a/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/SavedSitesSyncDataProvider.kt +++ b/saved-sites/saved-sites-impl/src/main/java/com/duckduckgo/savedsites/impl/sync/SavedSitesSyncDataProvider.kt @@ -133,7 +133,10 @@ class SavedSitesSyncDataProvider @Inject constructor( private fun getRequestEntriesFor( folderId: String, requestEntries: MutableList, + visitedFolderIds: MutableSet = mutableSetOf(), ): List { + // folder relations can form a loop, so a folder already on this branch is never descended into again + if (!visitedFolderIds.add(folderId)) return requestEntries val invalidItems = mutableListOf() syncSavedSitesRepository.getAllFolderContentSync(folderId).apply { val folder = repository.getFolder(folderId) @@ -156,7 +159,7 @@ class SavedSitesSyncDataProvider @Inject constructor( if (eachFolder.deleted != null) { requestEntries.add(deletedEntry(eachFolder.id)) } else { - getRequestEntriesFor(eachFolder.id, requestEntries) + getRequestEntriesFor(eachFolder.id, requestEntries, visitedFolderIds) } } requestEntries.add(encryptedFolder(fixFolderIfNecessary(folder)))