Desktop, Mobile: Harden failsafe logic to check for the presence of info.json, rather than just the item count (#11750)

packages/lib/Synchronizer.ts

@@ -22,7 +22,7 @@ import TaskQueue from './TaskQueue';
 import ItemUploader from './services/synchronizer/ItemUploader';
 import { FileApi, getSupportsDeltaWithItems, PaginatedList, RemoteItem } from './file-api';
 import JoplinDatabase from './JoplinDatabase';
-import { checkIfCanSync, fetchSyncInfo, getActiveMasterKey, localSyncInfo, mergeSyncInfos, saveLocalSyncInfo, setMasterKeyHasBeenUsed, SyncInfo, syncInfoEquals, uploadSyncInfo } from './services/synchronizer/syncInfoUtils';
+import { checkIfCanSync, fetchSyncInfo, checkSyncTargetIsValid, getActiveMasterKey, localSyncInfo, mergeSyncInfos, saveLocalSyncInfo, setMasterKeyHasBeenUsed, SyncInfo, syncInfoEquals, uploadSyncInfo } from './services/synchronizer/syncInfoUtils';
 import { getMasterPassword, setupAndDisableEncryption, setupAndEnableEncryption } from './services/e2ee/utils';
 import { generateKeyPair } from './services/e2ee/ppk';
 import syncDebugLog from './services/synchronizer/syncDebugLog';
@@ -859,6 +859,13 @@ export default class Synchronizer {
 						logger: logger,
 					});
 
+					// Ensure that if the sync target directory has changed, lost access, or has been purged by some external process while the sync is running, that a failsafe error is triggered where info.json and .sync/version.txt can no longer be found
+					// This check is more reliable than checking the count of items alone, as it is possible for sync items become segmented between 2 directories, possibly by the target directory changing during sync
+					// This scenario is possible with OneDrive sync, see https://github.com/laurent22/joplin/issues/11489
+					// This check while the sync is running is only necessary for the delta step of the sync, as this is where local deletions are calculated by comparing the local database and the sync target. These deletions are driven by the listResult field to determine which remote items exist
+					// As long as we check that info.json still exists after each time the listResult field is repopulated, there should not be a risk of unwanted deletions when failsafe is enabled, unless the target directory is directly manipulated by the user
+					await checkSyncTargetIsValid(this.api());
+
 					const supportsDeltaWithItems = getSupportsDeltaWithItems(listResult);
 
 					logger.info('supportsDeltaWithItems = ', supportsDeltaWithItems);

packages/lib/services/synchronizer/syncInfoUtils.test.ts

@@ -1,7 +1,9 @@
-import { afterAllCleanUp, setupDatabaseAndSynchronizer, logger, switchClient, encryptionService, msleep } from '../../testing/test-utils';
+import { afterAllCleanUp, setupDatabaseAndSynchronizer, logger, switchClient, encryptionService, msleep, fileApi } from '../../testing/test-utils';
 import MasterKey from '../../models/MasterKey';
-import { checkIfCanSync, localSyncInfo, masterKeyEnabled, mergeSyncInfos, saveLocalSyncInfo, setMasterKeyEnabled, SyncInfo, syncInfoEquals } from './syncInfoUtils';
+import { checkIfCanSync, localSyncInfo, masterKeyEnabled, mergeSyncInfos, saveLocalSyncInfo, setMasterKeyEnabled, SyncInfo, syncInfoEquals, checkSyncTargetIsValid, fetchSyncInfo } from './syncInfoUtils';
 import Setting from '../../models/Setting';
+import BaseItem from '../../models/BaseItem';
+import BaseModel from '../../models/BaseItem';
 import Logger from '@joplin/utils/Logger';
 
 describe('syncInfoUtils', () => {
@@ -9,6 +11,7 @@ describe('syncInfoUtils', () => {
 	beforeEach(async () => {
 		await setupDatabaseAndSynchronizer(1);
 		await switchClient(1);
+		await fileApi().clearRoot();
 	});
 
 	afterAll(async () => {
@@ -326,4 +329,67 @@ describe('syncInfoUtils', () => {
 
 		Logger.globalLogger.enabled = true;
 	});
+
+	it('should succeed when info.json exists for checkSyncTargetIsValid', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', true);
+		const syncInfo = new SyncInfo();
+		await fileApi().put('info.json', syncInfo.serialize());
+		expect(checkSyncTargetIsValid(fileApi())).resolves.not.toThrow();
+	}));
+
+	it('should succeed when info.json does not exist and failsafe is disabled for checkSyncTargetIsValid', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', false);
+		expect(checkSyncTargetIsValid(fileApi())).resolves.not.toThrow();
+	}));
+
+	it('should fail with failsafe error when info.json does not exist for checkSyncTargetIsValid', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', true);
+		await expect(checkSyncTargetIsValid(fileApi())).rejects.toThrow('Fail-safe: ');
+	}));
+
+	it('should succeed when info.json exists and is valid for fetchSyncInfo', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', true);
+		const expectedSyncInfo = new SyncInfo();
+		expectedSyncInfo.version = 50;
+		await fileApi().put('info.json', expectedSyncInfo.serialize());
+
+		const actualSyncInfo = await fetchSyncInfo(fileApi());
+		expect(actualSyncInfo).toStrictEqual(expectedSyncInfo);
+	}));
+
+	it('should fail with missing version error when info.json exists but is invalid for fetchSyncInfo', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', true);
+		await fileApi().put('info.json', new SyncInfo().serialize());
+		await expect(fetchSyncInfo(fileApi())).rejects.toThrow('Missing "version" field');
+	}));
+
+	it('should succeed when info.json does not exist but .sync/version.txt does exist for fetchSyncInfo', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', true);
+		await fileApi().put('.sync/version.txt', '{}');
+
+		const actualSyncInfo = await fetchSyncInfo(fileApi());
+		expect(actualSyncInfo.version).toBe(1);
+	}));
+
+	it('should succeed when info.json and .sync/version.txt does not exist and failsafe is disabled for fetchSyncInfo', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', false);
+
+		const actualSyncInfo = await fetchSyncInfo(fileApi());
+		expect(actualSyncInfo.version).toBe(0);
+	}));
+
+	it('should fail with failsafe error when info.json and .sync/version.txt does not exist when sync items are present for fetchSyncInfo', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', true);
+		const note = {
+			id: 1,
+			type_: BaseModel.TYPE_NOTE,
+		};
+		await BaseItem.saveSyncTime(fileApi().syncTargetId(), note, 1);
+		await expect(fetchSyncInfo(fileApi())).rejects.toThrow('Fail-safe: ');
+	}));
+
+	it('should succeed when info.json and .sync/version.txt does not exist when sync items are not present for fetchSyncInfo', (async () => {
+		Setting.setValue('sync.wipeOutFailSafe', true);
+		expect(fetchSyncInfo(fileApi())).resolves.not.toThrow();
+	}));
 });

packages/lib/services/synchronizer/syncInfoUtils.ts

@@ -2,6 +2,7 @@ import Logger from '@joplin/utils/Logger';
 import { FileApi } from '../../file-api';
 import JoplinDatabase from '../../JoplinDatabase';
 import Setting from '../../models/Setting';
+import BaseItem from '../../models/BaseItem';
 import { State } from '../../reducer';
 import { PublicPrivateKeyPair } from '../e2ee/ppk';
 import { MasterKeyEntity } from '../e2ee/types';
@@ -96,12 +97,44 @@ export async function fetchSyncInfo(api: FileApi): Promise<SyncInfo> {
 		// If info.json is not present, this might be an old sync target, in
 		// which case we can at least get the version number from version.txt
 		const oldVersion = await api.get('.sync/version.txt');
-		if (oldVersion) output = { version: 1 };
+
+		// Where info.json is missing, but .sync/version.txt is not, the sync target will be set as needing upgrade, and will be upgraded upon restarting the app
+		// If both info.json and .sync/version.txt are missing, it can be assumed that something has gone wrong with the sync target, so do not mark as needing upgrade and raise a failsafe error if not the initial sync
+		// When performing 'Delete local data and re-download from sync target' or 'Re-upload local data to sync target' actions, all sync_items are cleared down as if it were the initial sync
+		if (oldVersion) {
+			output = { version: 1 };
+		} else if (!(await isInitialSync(api.syncTargetId()))) {
+			throwFailsafeError();
+		}
 	}
 
 	return fixSyncInfo(new SyncInfo(JSON.stringify(output)));
 }
 
+export async function checkSyncTargetIsValid(api: FileApi): Promise<void> {
+	const syncTargetInfoText = await api.get('info.json');
+
+	if (!syncTargetInfoText) {
+		throwFailsafeError();
+	}
+}
+
+async function isInitialSync(syncTargetId: number) {
+	const syncedItems = await BaseItem.syncedItemIds(syncTargetId);
+	return syncedItems.length === 0;
+}
+
+// This failsafe validation producing this error will be performed regardless of which sync target is selected
+// Other failsafe validation is performed based on the percentage of items deleted in the "basicDelta" function
+// The basicDelta is not executed for all sync target types, but the validation in this function is superior at protecting against data loss
+// However it is still beneficial to keep the failsafe check which is driven by count of deleted items in place, as it can protect against deliberate deletion of all notes by the user,
+// where they are not aware of the implications of 2 way sync. This is just "nice to have" though, so would not be worth adding complexity to make it work for all sync target types
+function throwFailsafeError() {
+	if (Setting.value('sync.wipeOutFailSafe')) {
+		throw new JoplinError(_('Fail-safe: Sync was interrupted to prevent data loss, because the sync target is empty or damaged. To override this behaviour disable the fail-safe in the sync settings.'), 'failSafe');
+	}
+}
+
 export function saveLocalSyncInfo(syncInfo: SyncInfo) {
 	Setting.setValue('syncInfoCache', syncInfo.serialize());
 }