Desktop: Windows portable: Fix keychain-backed storage incorrectly enabled (#10942)

packages/lib/services/SettingUtils.ts

@@ -5,6 +5,7 @@ import Setting from '../models/Setting';
 import uuid from '../uuid';
 import { migrateLocalSyncInfo } from './synchronizer/syncInfoUtils';
 import KeychainServiceDriverBase from './keychain/KeychainServiceDriverBase';
+import shim from '../shim';
 
 type KeychainServiceDriverConstructor = new (appId: string, clientId: string)=> KeychainServiceDriverBase;
 
@@ -19,6 +20,14 @@ type KeychainServiceDriverConstructor = new (appId: string, clientId: string)=>
 export async function loadKeychainServiceAndSettings(keychainServiceDrivers: KeychainServiceDriverConstructor[]) {
 	const clientIdSetting = await Setting.loadOne('clientId');
 	const clientId = clientIdSetting ? clientIdSetting.value : uuid.create();
+
+	// Temporary workaround: For a short time, pre-release versions of Joplin Portable encrypted
+	// saved keys using the keychain. This can break sync when transferring Joplin between devices.
+	// To prevent secure keys from being lost, we enable read-only keychain access in portable mode.
+	if (shim.isPortable()) {
+		KeychainService.instance().readOnly = true;
+	}
+
 	await KeychainService.instance().initialize(
 		keychainServiceDrivers.map(Driver => new Driver(Setting.value('appId'), clientId)),
 	);

packages/lib/services/keychain/KeychainService.test.ts

@@ -13,18 +13,21 @@ interface SafeStorageMockOptions {
 }
 
 const mockSafeStorage = ({ // Safe storage
-	isEncryptionAvailable = jest.fn(() => true),
-	encryptString = jest.fn(async s => (`e:${s}`)),
-	decryptString = jest.fn(async s => s.substring(2)),
+	isEncryptionAvailable = () => true,
+	encryptString = async s => (`e:${s}`),
+	decryptString = async (s) => s.substring(2),
 }: SafeStorageMockOptions) => {
+	const mock = {
+		isEncryptionAvailable: jest.fn(isEncryptionAvailable),
+		encryptString: jest.fn(encryptString),
+		decryptString: jest.fn(decryptString),
+		getSelectedStorageBackend: jest.fn(() => 'mock'),
+	};
 	shim.electronBridge = () => ({
-		safeStorage: {
-			isEncryptionAvailable,
-			encryptString,
-			decryptString,
-			getSelectedStorageBackend: () => 'mock',
-		},
+		safeStorage: mock,
 	});
+
+	return mock;
 };
 
 const mockKeytar = () => {
@@ -51,10 +54,19 @@ const makeDrivers = () => [
 	new KeychainServiceDriverNode(Setting.value('appId'), Setting.value('clientId')),
 ];
 
+const testSaveLoadSecureSetting = async (expectedPassword: string) => {
+	Setting.setValue('encryption.masterPassword', expectedPassword);
+	await Setting.saveAll();
+
+	await Setting.load();
+	expect(Setting.value('encryption.masterPassword')).toBe(expectedPassword);
+};
+
 describe('KeychainService', () => {
 	beforeEach(async () => {
 		await setupDatabaseAndSynchronizer(0);
 		await switchClient(0);
+		KeychainService.instance().readOnly = false;
 		Setting.setValue('keychain.supported', 1);
 		shim.electronBridge = null;
 		shim.keytar = null;
@@ -91,16 +103,12 @@ describe('KeychainService', () => {
 		const keytarMock = mockKeytar();
 		await KeychainService.instance().initialize(makeDrivers());
 
-		Setting.setValue('encryption.masterPassword', 'test-password');
-		await Setting.saveAll();
+		await testSaveLoadSecureSetting('test-password');
 		expect(keytarMock.setPassword).toHaveBeenCalledWith(
 			`${Setting.value('appId')}.setting.encryption.masterPassword`,
 			`${Setting.value('clientId')}@joplin`,
 			'test-password',
 		);
-
-		await Setting.load();
-		expect(Setting.value('encryption.masterPassword')).toBe('test-password');
 	});
 
 	test('should re-check for keychain support when a new driver is added', async () => {
@@ -125,4 +133,57 @@ describe('KeychainService', () => {
 		await KeychainService.instance().detectIfKeychainSupported();
 		expect(Setting.value('keychain.supported')).toBe(0);
 	});
+
+	test('should load settings from a read-only KeychainService if not present in the database', async () => {
+		mockSafeStorage({});
+
+		const service = KeychainService.instance();
+		await service.initialize(makeDrivers());
+		expect(await service.setPassword('setting.encryption.masterPassword', 'keychain password')).toBe(true);
+
+		service.readOnly = true;
+		await service.initialize(makeDrivers());
+		await Setting.load();
+
+		expect(Setting.value('encryption.masterPassword')).toBe('keychain password');
+	});
+
+	test('settings should be saved to database with a read-only keychain', async () => {
+		const safeStorage = mockSafeStorage({});
+
+		const service = KeychainService.instance();
+		service.readOnly = true;
+
+		await service.initialize(makeDrivers());
+		await service.detectIfKeychainSupported();
+
+		expect(Setting.value('keychain.supported')).toBe(1);
+		await testSaveLoadSecureSetting('testing...');
+
+		expect(safeStorage.encryptString).not.toHaveBeenCalledWith('testing...');
+	});
+
+	test('loading settings with a read-only keychain should prefer the database', async () => {
+		const safeStorage = mockSafeStorage({});
+
+		const service = KeychainService.instance();
+		await service.initialize(makeDrivers());
+		// Set an initial value
+		expect(await service.setPassword('setting.encryption.masterPassword', 'test keychain password')).toBe(true);
+
+		service.readOnly = true;
+		await service.initialize(makeDrivers());
+
+		safeStorage.encryptString.mockClear();
+
+		Setting.setValue('encryption.masterPassword', 'test database password');
+		await Setting.saveAll();
+
+		await Setting.load();
+		expect(Setting.value('encryption.masterPassword')).toBe('test database password');
+		expect(await service.password('setting.encryption.masterPassword')).toBe('test keychain password');
+
+		// Should not have attempted to encrypt settings in read-only mode.
+		expect(safeStorage.encryptString).not.toHaveBeenCalled();
+	});
 });

packages/lib/services/keychain/KeychainService.ts

@@ -11,6 +11,7 @@ export default class KeychainService extends BaseService {
 	private keysNeedingMigration_: Set<string>;
 	private static instance_: KeychainService;
 	private enabled_ = true;
+	private readOnly_ = false;
 
 	public static instance(): KeychainService {
 		if (!this.instance_) this.instance_ = new KeychainService();
@@ -53,8 +54,17 @@ export default class KeychainService extends BaseService {
 		this.enabled_ = v;
 	}
 
+	public get readOnly() {
+		return this.readOnly_;
+	}
+
+	public set readOnly(v: boolean) {
+		this.readOnly_ = v;
+	}
+
 	public async setPassword(name: string, password: string): Promise<boolean> {
 		if (!this.enabled) return false;
+		if (this.readOnly_) return false;
 
 		// Optimization: Handles the case where the password doesn't need to change.
 		// TODO: Re-evaluate whether this optimization is necessary after refactoring the driver
@@ -137,16 +147,29 @@ export default class KeychainService extends BaseService {
 			Setting.setValue('keychain.supported', -1);
 		}
 
-		const passwordIsSet = await this.setPassword('zz_testingkeychain', 'mytest');
+		if (!this.readOnly) {
+			const passwordIsSet = await this.setPassword('zz_testingkeychain', 'mytest');
 
-		if (!passwordIsSet) {
-			this.logger().info('KeychainService: could not set test password - keychain support will be disabled');
-			Setting.setValue('keychain.supported', 0);
+			if (!passwordIsSet) {
+				this.logger().info('KeychainService: could not set test password - keychain support will be disabled');
+				Setting.setValue('keychain.supported', 0);
+			} else {
+				const result = await this.password('zz_testingkeychain');
+				await this.deletePassword('zz_testingkeychain');
+				this.logger().info('KeychainService: tried to set and get password. Result was:', result);
+				Setting.setValue('keychain.supported', result === 'mytest' ? 1 : 0);
+			}
 		} else {
-			const result = await this.password('zz_testingkeychain');
-			await this.deletePassword('zz_testingkeychain');
-			this.logger().info('KeychainService: tried to set and get password. Result was:', result);
-			Setting.setValue('keychain.supported', result === 'mytest' ? 1 : 0);
+			// The supported check requires write access to the keychain -- rely on the more
+			// limited support checks done by each driver.
+			const supported = this.drivers_.length > 0;
+			Setting.setValue('keychain.supported', supported ? 1 : 0);
+
+			if (supported) {
+				logger.info('Starting KeychainService in read-only mode. Keys will be read, but not written.');
+			} else {
+				logger.info('Failed to start in read-only mode -- no supported drivers found.');
+			}
 		}
 		Setting.setValue('keychain.lastAvailableDrivers', this.drivers_.map(driver => driver.driverId));
 	}

packages/lib/services/keychain/KeychainServiceDriver.node.ts

@@ -5,7 +5,7 @@ export default class KeychainServiceDriver extends KeychainServiceDriverBase {
 	public override readonly driverId: string = 'node-keytar';
 
 	public async supported(): Promise<boolean> {
-		return !!shim.keytar();
+		return !!shim.keytar?.();
 	}
 
 	public async setPassword(name: string, password: string): Promise<boolean> {

packages/lib/versionInfo.ts

@@ -76,7 +76,9 @@ export default function versionInfo(packageInfo: PackageInfo, plugins: Plugins)
 		_('Client ID: %s', Setting.value('clientId')),
 		_('Sync Version: %s', Setting.value('syncVersion')),
 		_('Profile Version: %s', reg.db().version()),
-		_('Keychain Supported: %s', Setting.value('keychain.supported') >= 1 ? _('Yes') : _('No')),
+		// The portable app temporarily supports read-only keychain access (but disallows
+		// write).
+		_('Keychain Supported: %s', (Setting.value('keychain.supported') >= 1 && !shim.isPortable()) ? _('Yes') : _('No')),
 	];
 
 	if (gitInfo) {