Merge pull request #1902 from floccusaddon/feat/addition-failsafe

feat(AdditionFailsafe): Extend failsafe mechanism to prevent creation of excessive amounts of bookmarks

Author: marcelklehr

_locales/en/messages.json

@@ -125,6 +125,9 @@
   "Error042": {
     "message": "E042: Remote bookmarks file size could not be retrieved. It is impossible to verify that the bookmarks file was downloaded in full. If this error persists please contact the server administrator."
   },
+  "Error043": {
+    "message": "E043: Failsafe: The current sync run would increase your bookmarks count by {0}%. Refusing to execute. Disable this failsafe in the profile settings if you want to proceed anyway."
+  },
   "LabelWebdavurl": {
     "message": "WebDAV URL"
   },
@@ -511,16 +514,16 @@
     "message": "Failsafe"
   },
   "DescriptionFailsafe": {
-    "message": "Sometimes configuration errors or software bugs can cause the unintended removal of data, which is then lost. To prevent that from happening, floccus will not delete more than 50% of your bookmarks in one go unless you disable this failsafe here."
+    "message": "Sometimes configuration errors or software bugs can cause the unintended removal of data, which is then lost, or the unintended duplication of data, which is then hard to sort out. To prevent this from happening, floccus will not delete more than 20% of your bookmarks in one go and will also not add more than 20% to your bookmarks count in one go, unless you disable this failsafe here."
   },
   "LabelFailsafeon": {
-    "message": "Enabled. Don't delete more than 50% of my local bookmarks without asking me first. (Recommended)"
+    "message": "Enabled. Will not delete or add more than 20% from/to your local bookmarks without asking you first. (Recommended)"
   },
   "LabelFailsafeoff": {
-    "message": "Disabled. Allow the removal of more than 50% of my local bookmarks without confirming with me."
+    "message": "Disabled. Will allow removing or adding more than 20% from/to your local bookmarks without confirming with you."
   },
   "StatusFailsafeoff": {
-    "message": "Failsafe disabled. You are at risk for unintended data loss. It is recommended to enable the failsafe in the profile settings."
+    "message": "Failsafe disabled. You are at risk for unintended data loss or duplication. It is recommended to enable the failsafe in the profile settings."
   },
   "LabelAdaptergoogledrive": {
     "message": "Google Drive"

src/errors/Error.ts

@@ -240,14 +240,14 @@ export class InterruptedSyncError extends FloccusError {
 
 // code 28 is unused
 
-export class FailsafeError extends FloccusError {
+export class DeletionFailsafeError extends FloccusError {
   public percent: number
 
   constructor(percent:number) {
     super(`E029: Failsafe: The current sync run would delete ${percent}% of your bookmarks. Refusing to execute. Disable this failsafe in the profile settings if you want to proceed anyway.`)
     this.code = 29
     this.percent = percent
-    Object.setPrototypeOf(this, FailsafeError.prototype)
+    Object.setPrototypeOf(this, DeletionFailsafeError.prototype)
   }
 }
 
@@ -357,4 +357,15 @@ export class FileSizeUnknown extends FloccusError {
     this.code = 42
     Object.setPrototypeOf(this, FileSizeUnknown.prototype)
   }
+}
+
+export class AdditionFailsafeError extends FloccusError {
+  public percent: number
+
+  constructor(percent:number) {
+    super(`E043: Failsafe: The current sync run would increase your bookmarks count by ${percent}%. Refusing to execute. Disable this failsafe in the profile settings if you want to proceed anyway.`)
+    this.code = 43
+    this.percent = percent
+    Object.setPrototypeOf(this, AdditionFailsafeError.prototype)
+  }
 }

src/lib/browser/BrowserAccount.ts

@@ -4,8 +4,9 @@ import browser from '../browser-api'
 import AdapterFactory from '../AdapterFactory'
 import Account from '../Account'
 import {
+  AdditionFailsafeError,
   CreateBookmarkError,
-  FailsafeError, FloccusError,
+  DeletionFailsafeError, FloccusError,
   HttpError,
   InconsistentBookmarksExistenceError, LockFileError,
   MissingItemOrderError,
@@ -106,7 +107,10 @@ export default class BrowserAccount extends Account {
     if (er instanceof LockFileError) {
       return i18n.getMessage('Error' + String(er.code).padStart(3, '0'), [er.status, er.lockFile])
     }
-    if (er instanceof FailsafeError) {
+    if (er instanceof DeletionFailsafeError) {
+      return i18n.getMessage('Error' + String(er.code).padStart(3, '0'), [er.percent])
+    }
+    if (er instanceof AdditionFailsafeError) {
       return i18n.getMessage('Error' + String(er.code).padStart(3, '0'), [er.percent])
     }
     if (er instanceof CreateBookmarkError) {

src/lib/native/NativeAccount.ts

@@ -4,8 +4,9 @@ import AdapterFactory from '../AdapterFactory'
 import Account from '../Account'
 import { IAccountData } from '../interfaces/AccountStorage'
 import {
+  AdditionFailsafeError,
   CreateBookmarkError,
-  FailsafeError, FloccusError,
+  DeletionFailsafeError, FloccusError,
   HttpError,
   InconsistentBookmarksExistenceError, LockFileError,
   MissingItemOrderError,
@@ -84,7 +85,10 @@ export default class NativeAccount extends Account {
     if (er instanceof LockFileError) {
       return i18n.getMessage('Error' + String(er.code).padStart(3, '0'), [er.status, er.lockFile])
     }
-    if (er instanceof FailsafeError) {
+    if (er instanceof DeletionFailsafeError) {
+      return i18n.getMessage('Error' + String(er.code).padStart(3, '0'), [er.percent])
+    }
+    if (er instanceof AdditionFailsafeError) {
       return i18n.getMessage('Error' + String(er.code).padStart(3, '0'), [er.percent])
     }
     if (er instanceof CreateBookmarkError) {

src/lib/strategies/Default.ts

@@ -23,7 +23,7 @@ import { throttle } from 'throttle-debounce'
 import Mappings, { MappingSnapshot } from '../Mappings'
 import TResource, { OrderFolderResource, TLocalTree } from '../interfaces/Resource'
 import { TAdapter } from '../interfaces/Adapter'
-import { CancelledSyncError, FailsafeError } from '../../errors/Error'
+import { AdditionFailsafeError, CancelledSyncError, DeletionFailsafeError } from '../../errors/Error'
 
 import NextcloudBookmarksAdapter from '../adapters/NextcloudBookmarks'
 import CachingAdapter from '../adapters/Caching'
@@ -246,7 +246,10 @@ export default class SyncProcess {
 
     Logger.log({localPlan: this.localPlanStage2, serverPlan: this.serverPlanStage2})
 
-    this.applyFailsafe(this.localPlanStage2.REMOVE)
+    this.applyDeletionFailsafe(this.localTreeRoot, this.localPlanStage2.REMOVE)
+    this.applyAdditionFailsafe(this.localTreeRoot, this.localPlanStage2.CREATE)
+    this.applyDeletionFailsafe(this.serverTreeRoot, this.serverPlanStage2.REMOVE)
+    this.applyAdditionFailsafe(this.serverTreeRoot, this.serverPlanStage2.CREATE)
 
     if (!this.localDonePlan) {
       this.localDonePlan = {
@@ -360,16 +363,30 @@ export default class SyncProcess {
     this.serverTreeRoot.createIndex()
   }
 
-  protected applyFailsafe(removals: Diff<TItemLocation, TItemLocation, RemoveAction<TItemLocation, TItemLocation>>) {
-    const localCountTotal = this.localTreeRoot.count()
-    const localCountDeleted = removals.getActions().reduce((count, action) => count + action.payload.count(), 0)
+  protected applyDeletionFailsafe(tree: Folder<TItemLocation>, removals: Diff<TItemLocation, TItemLocation, RemoveAction<TItemLocation, TItemLocation>>) {
+    const countTotal = tree.count()
+    const countDeleted = removals.getActions().reduce((count, action) => count + action.payload.count(), 0)
 
-    Logger.log('Checking failsafe: ' + localCountDeleted + '/' + localCountTotal + '=' + (localCountDeleted / localCountTotal))
+    Logger.log('Checking deletion failsafe: ' + countDeleted + '/' + countTotal + '=' + (countDeleted / countTotal))
     // Failsafe kicks in if more than 20% is deleted or more than 1k bookmarks
-    if ((localCountTotal > 5 && localCountDeleted / localCountTotal > 0.2) || localCountDeleted > 1000) {
+    if ((countTotal > 5 && countDeleted / countTotal > 0.2) || countDeleted > 1000) {
       const failsafe = this.server.getData().failsafe
       if (failsafe !== false || typeof failsafe === 'undefined') {
-        throw new FailsafeError(Math.ceil((localCountDeleted / localCountTotal) * 100))
+        throw new DeletionFailsafeError(Math.ceil((countDeleted / countTotal) * 100))
+      }
+    }
+  }
+
+  protected applyAdditionFailsafe(tree: Folder<TItemLocation>, creations: Diff<TItemLocation, TItemLocation, CreateAction<TItemLocation, TItemLocation>>) {
+    const countTotal = tree.count()
+    const countAdded = creations.getActions().reduce((count, action) => count + action.payload.count(), 0)
+
+    Logger.log('Checking addition failsafe: ' + countAdded + '/' + countTotal + '=' + (countAdded / countTotal))
+    // Failsafe kicks in if more than 20% is added or more than 1k bookmarks
+    if (countTotal > 5 && (countAdded / countTotal > 0.2 || countAdded > 1000)) {
+      const failsafe = this.server.getData().failsafe
+      if (failsafe !== false || typeof failsafe === 'undefined') {
+        throw new AdditionFailsafeError(Math.ceil((countAdded / countTotal) * 100))
       }
     }
   }

src/lib/strategies/Unidirectional.ts

@@ -158,7 +158,11 @@ export default class UnidirectionalSyncProcess extends DefaultStrategy {
     this.actionsPlanned = Object.values(this.revertPlan).reduce((acc, diff) => diff.getActions().length + acc, 0)
 
     if (this.direction === ItemLocation.LOCAL) {
-      this.applyFailsafe(this.revertPlan.REMOVE)
+      this.applyDeletionFailsafe(this.localTreeRoot, this.revertPlan.REMOVE)
+      this.applyAdditionFailsafe(this.localTreeRoot, this.revertPlan.CREATE)
+    } else {
+      this.applyDeletionFailsafe(this.serverTreeRoot, this.revertPlan.REMOVE)
+      this.applyAdditionFailsafe(this.serverTreeRoot, this.revertPlan.CREATE)
     }
 
     if (this.canceled) {

src/test/test.js

@@ -11,6 +11,7 @@ import DefunctCrypto from '../lib/DefunctCrypto'
 import Controller from '../lib/Controller'
 import FakeAdapter from '../lib/adapters/Fake'
 import BrowserTree from '../lib/browser/BrowserTree'
+import { AdditionFailsafeError, DeletionFailsafeError } from '../errors/Error'
 
 chai.use(chaiAsPromised)
 const expect = chai.expect
@@ -2174,7 +2175,7 @@ describe('Floccus', function() {
             expect(localTree.findBookmark(bookmark1.id)).to.be.ok
             expect(localTree.findBookmark(bookmark2.id)).to.be.ok
           })
-          it('should error when deleting too much local data', async function() {
+          it('should error when deleting too much local data (failsafe)', async function() {
             if (ACCOUNT_DATA.noCache) {
               this.skip()
             }
@@ -2188,6 +2189,7 @@ describe('Floccus', function() {
               title: 'bar',
               parentId: fooFolder.id
             })
+            // We need more than 5 bookmarks to be above the negligibility threshold
             await Promise.all([
               'http://ur.l/',
               'http://ur.ll/',
@@ -2222,6 +2224,176 @@ describe('Floccus', function() {
 
             await account.sync()
             expect(account.getData().error).to.be.ok // should have errored
+            expect(account.getData().error).to.include((new DeletionFailsafeError).code)
+          })
+          it('should error when adding too much local data (failsafe)', async function() {
+            if (ACCOUNT_DATA.noCache) {
+              this.skip()
+            }
+
+            const localRoot = account.getData().localRoot
+            const fooFolder = await browser.bookmarks.create({
+              title: 'foo',
+              parentId: localRoot
+            })
+            const barFolder = await browser.bookmarks.create({
+              title: 'bar',
+              parentId: fooFolder.id
+            })
+            // Create 6 bookmarks to be above the negligibility threshold
+            await Promise.all([
+              'http://ur.1l/',
+              'http://ur2.l/',
+              'http://ur3.l/',
+              'http://ur4.l/',
+              'http://ur5.l/',
+              'http://ur6.l/'
+            ].map(url => browser.bookmarks.create({
+              title: 'url',
+              url,
+              parentId: barFolder.id
+            })))
+            await account.sync()
+            expect(account.getData().error).to.not.be.ok
+
+            // Simulate an extreme increase of bookmarks
+            const tree = await getAllBookmarks(account)
+            await withSyncConnection(account, async() => {
+              await Promise.all([
+                'http://ur.1l/',
+                'http://ur2.l/',
+                'http://ur3.l/',
+                'http://ur4.l/',
+                'http://ur5.l/',
+                'http://ur6.l/',
+                'http://ur7.l/',
+                'http://ur8.l/',
+                'http://ur9.l/',
+                'http://ur10.l/',
+                'http://ur.11l/',
+                'http://ur12.l/',
+                'http://ur13.l/',
+                'http://ur14.l/',
+                'http://ur15.l/',
+                'http://ur16.l/',
+                'http://ur17.l/',
+                'http://ur18.l/',
+                'http://ur19.l/',
+                'http://ur20.l/',
+              ].map(url => account.server.createBookmark(new Bookmark({
+                title: 'url',
+                url,
+                parentId: tree.id
+              }))))
+            })
+
+            await account.sync()
+            expect(account.getData().error).to.be.ok // should have errored
+            expect(account.getData().error).to.include((new AdditionFailsafeError).code)
+          })
+          it('should error when deleting too much remote data (failsafe)', async function() {
+            if (ACCOUNT_DATA.noCache) {
+              this.skip()
+            }
+
+            const localRoot = account.getData().localRoot
+            const fooFolder = await browser.bookmarks.create({
+              title: 'foo',
+              parentId: localRoot
+            })
+            const barFolder = await browser.bookmarks.create({
+              title: 'bar',
+              parentId: fooFolder.id
+            })
+            // We need more than 5 bookmarks to be above the negligibility threshold
+            await Promise.all([
+              'http://ur.l/',
+              'http://ur.ll/',
+              'http://ur2.l/',
+              'http://ur3.l/',
+              'http://ur4.l/',
+              'http://ur5.l/',
+              'http://ur6.l/',
+              'http://ur7.l/',
+              'http://ur8.l/',
+              'http://ur9.l/',
+              'http://ur10.l/',
+            ].map(url => browser.bookmarks.create({
+              title: 'url',
+              url,
+              parentId: barFolder.id
+            })))
+            await account.sync()
+            expect(account.getData().error).to.not.be.ok
+
+            // Remove everything
+            await browser.bookmarks.removeTree(barFolder.id)
+
+            await account.sync()
+            expect(account.getData().error).to.be.ok // should have errored
+            expect(account.getData().error).to.include((new DeletionFailsafeError).code)
+          })
+          it('should error when adding too much remote data (failsafe)', async function() {
+            if (ACCOUNT_DATA.noCache) {
+              this.skip()
+            }
+
+            const localRoot = account.getData().localRoot
+            const fooFolder = await browser.bookmarks.create({
+              title: 'foo',
+              parentId: localRoot
+            })
+            const barFolder = await browser.bookmarks.create({
+              title: 'bar',
+              parentId: fooFolder.id
+            })
+            // Create 6 bookmarks to be above the negligibility threshold
+            await Promise.all([
+              'http://ur.1l/',
+              'http://ur2.l/',
+              'http://ur3.l/',
+              'http://ur4.l/',
+              'http://ur5.l/',
+              'http://ur6.l/'
+            ].map(url => browser.bookmarks.create({
+              title: 'url',
+              url,
+              parentId: barFolder.id
+            })))
+            await account.sync()
+            expect(account.getData().error).to.not.be.ok
+
+            // Simulate an extreme increase of bookmarks
+            await Promise.all([
+              'http://ur.1l/',
+              'http://ur2.l/',
+              'http://ur3.l/',
+              'http://ur4.l/',
+              'http://ur5.l/',
+              'http://ur6.l/',
+              'http://ur7.l/',
+              'http://ur8.l/',
+              'http://ur9.l/',
+              'http://ur10.l/',
+              'http://ur.11l/',
+              'http://ur12.l/',
+              'http://ur13.l/',
+              'http://ur14.l/',
+              'http://ur15.l/',
+              'http://ur16.l/',
+              'http://ur17.l/',
+              'http://ur18.l/',
+              'http://ur19.l/',
+              'http://ur20.l/',
+            ].map(url => browser.bookmarks.create({
+              title: 'url',
+              url,
+              parentId: barFolder.id
+            })))
+
+            await account.sync()
+            expect(account.getData().error).to.be.ok // should have errored
+            expect(account.getData().error).to.include((new AdditionFailsafeError).code)
           })
           it('should leave alone unaccepted bookmarks entirely', async function() {
             if (!~ACCOUNT_DATA.type.indexOf('nextcloud')) {