Desktop: Resolves #11992: Multiple instances: Secure local server (#11999)

.eslintignore

@@ -1073,6 +1073,8 @@ packages/lib/fs-driver-base.js
 packages/lib/fs-driver-node.js
 packages/lib/fsDriver.test.js
 packages/lib/geolocation-node.js
+packages/lib/getAppName.test.js
+packages/lib/getAppName.js
 packages/lib/hooks/useAsyncEffect.js
 packages/lib/hooks/useElementSize.js
 packages/lib/hooks/useEventListener.js

.gitignore

@@ -1048,6 +1048,8 @@ packages/lib/fs-driver-base.js
 packages/lib/fs-driver-node.js
 packages/lib/fsDriver.test.js
 packages/lib/geolocation-node.js
+packages/lib/getAppName.test.js
+packages/lib/getAppName.js
 packages/lib/hooks/useAsyncEffect.js
 packages/lib/hooks/useElementSize.js
 packages/lib/hooks/useEventListener.js

packages/app-desktop/ElectronAppWrapper.ts

@@ -21,6 +21,8 @@ import { clearTimeout, setTimeout } from 'timers';
 import { resolve } from 'path';
 import { defaultWindowId } from '@joplin/lib/reducer';
 import { msleep, Second } from '@joplin/utils/time';
+import determineBaseAppDirs from '@joplin/lib/determineBaseAppDirs';
+import getAppName from '@joplin/lib/getAppName';
 
 interface RendererProcessQuitReply {
 	canClose: boolean;
@@ -579,7 +581,11 @@ export default class ElectronAppWrapper {
 
 		if (port === null) port = this.ipcStartPort_;
 
-		return await sendMessage(port, { ...message, sourcePort: this.ipcServer_.port }, {
+		return await sendMessage(port, {
+			...message,
+			sourcePort: this.ipcServer_.port,
+			secretKey: this.ipcServer_.secretKey,
+		}, {
 			logger: this.ipcLogger_,
 			...options,
 		});
@@ -631,6 +637,7 @@ export default class ElectronAppWrapper {
 						const response = await this.sendCrossAppIpcMessage({
 							action: 'ping',
 							data: null,
+							secretKey: this.ipcServer_.secretKey,
 						}, message.sourcePort, {
 							sendToSpecificPortOnly: true,
 						});
@@ -662,7 +669,12 @@ export default class ElectronAppWrapper {
 			},
 		};
 
-		this.ipcServer_ = await startServer(this.ipcStartPort_, async (message) => {
+		const defaultProfileDir = determineBaseAppDirs('', getAppName(true, this.env() === 'dev'), '').rootProfileDir;
+		const secretKeyFilePath = `${defaultProfileDir}/ipc_secret_key.txt`;
+
+		this.ipcLogger_.info('Starting server using secret key:', secretKeyFilePath);
+
+		this.ipcServer_ = await startServer(this.ipcStartPort_, secretKeyFilePath, async (message) => {
 			if (messageHandlers[message.action]) {
 				this.ipcLogger_.info('Got message:', message);
 				return messageHandlers[message.action](message);
@@ -684,6 +696,7 @@ export default class ElectronAppWrapper {
 				profilePath: this.profilePath_,
 				argv: process.argv,
 			},
+			secretKey: this.ipcServer_.secretKey,
 		};
 
 		await this.sendCrossAppIpcMessage(message);

packages/lib/BaseApplication.ts

@@ -65,6 +65,7 @@ import processStartFlags from './utils/processStartFlags';
 import { setupAutoDeletion } from './services/trash/permanentlyDeleteOldItems';
 import determineProfileAndBaseDir from './determineBaseAppDirs';
 import NavService from './services/NavService';
+import getAppName from './getAppName';
 
 const appLogger: LoggerWrapper = Logger.create('App');
 
@@ -679,8 +680,7 @@ export default class BaseApplication {
 
 		let appName = options.appName;
 		if (!appName) {
-			appName = initArgs.env === 'dev' ? 'joplindev' : 'joplin';
+			appName = getAppName(Setting.value('appId').indexOf('-desktop') >= 0, initArgs.env === 'dev');
-			if (Setting.value('appId').indexOf('-desktop') >= 0) appName += '-desktop';
 		}
 		Setting.setConstant('appName', appName);
 

packages/lib/getAppName.test.ts

@@ -0,0 +1,12 @@
+import getAppName from './getAppName';
+
+describe('getAppName', () => {
+
+	it('should get the app name', () => {
+		expect(getAppName(true, true)).toBe('joplindev-desktop');
+		expect(getAppName(true, false)).toBe('joplin-desktop');
+		expect(getAppName(false, false)).toBe('joplin');
+		expect(getAppName(false, true)).toBe('joplindev');
+	});
+
+});

packages/lib/getAppName.ts

@@ -0,0 +1,5 @@
+export default (isDesktop: boolean, isDev: boolean) => {
+	let appName = isDev ? 'joplindev' : 'joplin';
+	if (isDesktop) appName += '-desktop';
+	return appName;
+};

packages/utils/crypto.ts

@@ -0,0 +1,9 @@
+/* eslint-disable import/prefer-default-export */
+
+import { randomBytes } from 'crypto';
+
+export const getSecureRandomString = (length: number): string => {
+	const bytes = randomBytes(Math.ceil(length * 2));
+	const randomString = bytes.toString('base64').replace(/[^a-zA-Z0-9]/g, '');
+	return randomString.slice(0, length);
+};

packages/utils/fs.test.ts

@@ -1,10 +1,12 @@
+/* eslint-disable import/prefer-default-export */
+
 import { mkdirp } from 'fs-extra';
 import { FileLocker } from './fs';
 import { msleep, Second } from './time';
 
 const baseTempDir = `${__dirname}/../app-cli/tests/tmp`;
 
-const createTempDir = async () => {
+export const createTempDir = async () => {
 	const p = `${baseTempDir}/${Date.now()}`;
 	await mkdirp(p);
 	return p;

packages/utils/ipc.test.ts

@@ -1,11 +1,15 @@
+import { readFile } from 'fs/promises';
+import { createTempDir } from './fs.test';
 import { newHttpError, sendMessage, startServer, stopServer } from './ipc';
 
 describe('ipc', () => {
 
 	it('should send and receive messages', async () => {
+		const tempDir = await createTempDir();
+		const secretFilePath = `${tempDir}/secret.txt`;
 		const startPort = 41168;
 
-		const server1 = await startServer(startPort, async (request) => {
+		const server1 = await startServer(startPort, secretFilePath, async (request) => {
 			if (request.action === 'testing') {
 				return {
 					text: 'hello1',
@@ -15,7 +19,7 @@ describe('ipc', () => {
 			throw newHttpError(404);
 		});
 
-		const server2 = await startServer(startPort, async (request) => {
+		const server2 = await startServer(startPort, secretFilePath, async (request) => {
 			if (request.action === 'testing') {
 				return {
 					text: 'hello2',
@@ -31,12 +35,15 @@ describe('ipc', () => {
 			throw newHttpError(404);
 		});
 
+		const secretKey = await readFile(secretFilePath, 'utf-8');
+
 		{
 			const responses = await sendMessage(startPort, {
 				action: 'testing',
 				data: {
 					test: 1234,
 				},
+				secretKey,
 			});
 
 			expect(responses).toEqual([
@@ -49,6 +56,7 @@ describe('ipc', () => {
 			const responses = await sendMessage(startPort, {
 				action: 'ping',
 				data: null,
+				secretKey,
 			});
 
 			expect(responses).toEqual([
@@ -63,6 +71,7 @@ describe('ipc', () => {
 					test: 1234,
 				},
 				sourcePort: 41168,
+				secretKey,
 			});
 
 			expect(responses).toEqual([
@@ -74,4 +83,44 @@ describe('ipc', () => {
 		await stopServer(server2);
 	});
 
+	it('should not process message if secret is invalid', async () => {
+		const tempDir = await createTempDir();
+		const secretFilePath = `${tempDir}/secret.txt`;
+		const startPort = 41168;
+
+		const server = await startServer(startPort, secretFilePath, async (request) => {
+			if (request.action === 'testing') {
+				return {
+					text: 'hello1',
+				};
+			}
+
+			throw newHttpError(404);
+		});
+
+		const secretKey = await readFile(secretFilePath, 'utf-8');
+
+		{
+			const responses = await sendMessage(startPort, {
+				action: 'testing',
+				data: null,
+				secretKey: 'wrong_key',
+			});
+
+			expect(responses.length).toBe(0);
+		}
+
+		{
+			const responses = await sendMessage(startPort, {
+				action: 'testing',
+				data: null,
+				secretKey,
+			});
+
+			expect(responses.length).toBe(1);
+		}
+
+		await stopServer(server);
+	});
+
 });

packages/utils/ipc.ts

@@ -2,6 +2,9 @@ import { createServer, IncomingMessage, ServerResponse } from 'http';
 import fetch from 'node-fetch';
 import { Server } from 'http';
 import Logger from './Logger';
+import { pathExists } from 'fs-extra';
+import { readFile, writeFile } from 'fs/promises';
+import { getSecureRandomString } from './crypto';
 
 const tcpPortUsed = require('tcp-port-used');
 const maxPorts = 10;
@@ -51,6 +54,7 @@ export interface Message {
 	action: string;
 	data: object|number|string|null;
 	sourcePort?: number;
+	secretKey?: string;
 }
 
 type Response = string|number|object|boolean;
@@ -66,21 +70,52 @@ export type IpcMessageHandler = (message: Message)=> Promise<Response|void>;
 export interface IpcServer {
 	port: number;
 	httpServer: Server;
+	secretKey: string;
 }
 
 interface StartServerOptions {
 	logger?: Logger;
 }
 
-export const startServer = async (startPort: number, messageHandler: IpcMessageHandler, options: StartServerOptions|null = null): Promise<IpcServer> => {
+const getSecretKey = async (filePath: string) => {
+	try {
+		const keyLength = 64;
+
+		const writeKeyToFile = async () => {
+			const key = getSecureRandomString(keyLength);
+			await writeFile(filePath, key, 'utf-8');
+			return key;
+		};
+
+		if (!(await pathExists(filePath))) {
+			return await writeKeyToFile();
+		}
+
+		const key = await readFile(filePath, 'utf-8');
+		if (key.length !== keyLength) return await writeKeyToFile();
+
+		return key;
+	} catch (error) {
+		const e = error as NodeJS.ErrnoException;
+		e.message = `Could not get secret key from file: ${filePath}`;
+		throw e;
+	}
+};
+
+// `secretKeyFilePath` must be the same for all the instances that can communicate with each others
+export const startServer = async (startPort: number, secretKeyFilePath: string, messageHandler: IpcMessageHandler, options: StartServerOptions|null = null): Promise<IpcServer> => {
 	const port = await findAvailablePort(startPort);
 	const logger = options && options.logger ? options.logger : new Logger();
 
+	const secretKey = await getSecretKey(secretKeyFilePath);
+
 	return new Promise<IpcServer>((resolve, reject) => {
 		try {
 			const server = createServer(async (req: IncomingMessage, res: ServerResponse) => {
+				let message: Message|null = null;
 				try {
-					const message = await parseJson(req) as Message;
+					message = await parseJson(req) as Message;
+					if (message.secretKey !== secretKey) throw newHttpError(401, 'Invalid secret key');
 					if (!message.action) throw newHttpError(400, 'Missing "action" property in message');
 					const response = await messageHandler(message);
 					res.writeHead(200, { 'Content-Type': 'application/json' });
@@ -88,6 +123,7 @@ export const startServer = async (startPort: number, messageHandler: IpcMessageH
 				} catch (error) {
 					const httpError = error as HttpError;
 					const httpCode = httpError.httpCode || 500;
+					logger.error('Could not response to request:', message, 'Error', httpCode, httpError.message);
 					res.writeHead(httpCode, { 'Content-Type': 'text/plain' });
 					res.end(`Error ${httpCode}: ${httpError.message}`);
 				}
@@ -101,6 +137,7 @@ export const startServer = async (startPort: number, messageHandler: IpcMessageH
 				resolve({
 					httpServer: server,
 					port,
+					secretKey,
 				});
 			});
 		} catch (error) {

readme/dev/spec/multiple_instances.md

@@ -14,6 +14,8 @@ Joplin Desktop supports multiple instances through **profile locking** and **IPC
 
 - When a message is sent, the implementation automatically discovers running IPC servers.
 
+- Messages are secured using a secret that is shared by all applications. That secret is read from the profile directory of the main instance. It is created by the server if it doesn't exist. This ensures that, for example, a browser cannot send a valid message to the apps.
+
 ### Instance Differentiation
 
 - The `--alt-instance-id` flag must be used to launch an alternative instance. This disables services like the Web Clipper.