Desktop: Resolves #10424: Move the note viewer to a separate process (#10678)

.eslintignore

@@ -500,6 +500,10 @@ packages/app-desktop/utils/7zip/pathToBundled7Zip.js
 packages/app-desktop/utils/checkForUpdatesUtils.test.js
 packages/app-desktop/utils/checkForUpdatesUtils.js
 packages/app-desktop/utils/checkForUpdatesUtilsTestData.js
+packages/app-desktop/utils/customProtocols/constants.js
+packages/app-desktop/utils/customProtocols/handleCustomProtocols.test.js
+packages/app-desktop/utils/customProtocols/handleCustomProtocols.js
+packages/app-desktop/utils/customProtocols/registerCustomProtocols.js
 packages/app-desktop/utils/isSafeToOpen.test.js
 packages/app-desktop/utils/isSafeToOpen.js
 packages/app-desktop/utils/markupLanguageUtils.js
@@ -1280,6 +1284,8 @@ packages/lib/utils/joplinCloud/types.js
 packages/lib/utils/processStartFlags.js
 packages/lib/utils/replaceUnsupportedCharacters.test.js
 packages/lib/utils/replaceUnsupportedCharacters.js
+packages/lib/utils/resolvePathWithinDir.test.js
+packages/lib/utils/resolvePathWithinDir.js
 packages/lib/utils/userFetcher.js
 packages/lib/utils/webDAVUtils.test.js
 packages/lib/utils/webDAVUtils.js

.gitignore

@@ -479,6 +479,10 @@ packages/app-desktop/utils/7zip/pathToBundled7Zip.js
 packages/app-desktop/utils/checkForUpdatesUtils.test.js
 packages/app-desktop/utils/checkForUpdatesUtils.js
 packages/app-desktop/utils/checkForUpdatesUtilsTestData.js
+packages/app-desktop/utils/customProtocols/constants.js
+packages/app-desktop/utils/customProtocols/handleCustomProtocols.test.js
+packages/app-desktop/utils/customProtocols/handleCustomProtocols.js
+packages/app-desktop/utils/customProtocols/registerCustomProtocols.js
 packages/app-desktop/utils/isSafeToOpen.test.js
 packages/app-desktop/utils/isSafeToOpen.js
 packages/app-desktop/utils/markupLanguageUtils.js
@@ -1259,6 +1263,8 @@ packages/lib/utils/joplinCloud/types.js
 packages/lib/utils/processStartFlags.js
 packages/lib/utils/replaceUnsupportedCharacters.test.js
 packages/lib/utils/replaceUnsupportedCharacters.js
+packages/lib/utils/resolvePathWithinDir.test.js
+packages/lib/utils/resolvePathWithinDir.js
 packages/lib/utils/userFetcher.js
 packages/lib/utils/webDAVUtils.test.js
 packages/lib/utils/webDAVUtils.js

packages/app-desktop/ElectronAppWrapper.ts

@@ -1,4 +1,4 @@
-import Logger from '@joplin/utils/Logger';
+import Logger, { LoggerWrapper } from '@joplin/utils/Logger';
 import { PluginMessage } from './services/plugins/PluginRunner';
 import shim from '@joplin/lib/shim';
 import { isCallbackUrl } from '@joplin/lib/callbackUrlUtils';
@@ -13,6 +13,7 @@ const fs = require('fs-extra');
 import { dialog, ipcMain } from 'electron';
 import { _ } from '@joplin/lib/locale';
 import restartInSafeModeFromMain from './utils/restartInSafeModeFromMain';
+import handleCustomProtocols, { CustomProtocolHandler } from './utils/customProtocols/handleCustomProtocols';
 import { clearTimeout, setTimeout } from 'timers';
 
 interface RendererProcessQuitReply {
@@ -40,6 +41,7 @@ export default class ElectronAppWrapper {
 	private rendererProcessQuitReply_: RendererProcessQuitReply = null;
 	private pluginWindows_: PluginWindows = {};
 	private initialCallbackUrl_: string = null;
+	private customProtocolHandler_: CustomProtocolHandler = null;
 
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 	public constructor(electronApp: any, env: string, profilePath: string|null, isDebugMode: boolean, initialCallbackUrl: string) {
@@ -454,6 +456,14 @@ export default class ElectronAppWrapper {
 		return false;
 	}
 
+	public initializeCustomProtocolHandler(logger: LoggerWrapper) {
+		this.customProtocolHandler_ ??= handleCustomProtocols(logger);
+	}
+
+	public getCustomProtocolHandler() {
+		return this.customProtocolHandler_;
+	}
+
 	public async start() {
 		// Since we are doing other async things before creating the window, we might miss
 		// the "ready" event. So we use the function below to make sure that the app is ready.

packages/app-desktop/app.ts

@@ -71,6 +71,7 @@ import OcrService from '@joplin/lib/services/ocr/OcrService';
 import OcrDriverTesseract from '@joplin/lib/services/ocr/drivers/OcrDriverTesseract';
 import SearchEngine from '@joplin/lib/services/search/SearchEngine';
 import { PackageInfo } from '@joplin/lib/versionInfo';
+import { CustomProtocolHandler } from './utils/customProtocols/handleCustomProtocols';
 import { refreshFolders } from '@joplin/lib/folders-screen-utils';
 
 const pluginClasses = [
@@ -88,6 +89,7 @@ class Application extends BaseApplication {
 	private checkAllPluginStartedIID_: any = null;
 	private initPluginServiceDone_ = false;
 	private ocrService_: OcrService;
+	private protocolHandler_: CustomProtocolHandler;
 
 	public constructor() {
 		super();
@@ -167,6 +169,12 @@ class Application extends BaseApplication {
 			this.handleThemeAutoDetect();
 		}
 
+		if (action.type === 'PLUGIN_ADD') {
+			const plugin = PluginService.instance().pluginById(action.plugin.id);
+			this.protocolHandler_.allowReadAccessToDirectory(plugin.baseDir);
+			this.protocolHandler_.allowReadAccessToDirectory(plugin.dataDir);
+		}
+
 		return result;
 	}
 
@@ -427,6 +435,20 @@ class Application extends BaseApplication {
 			bridge().openDevTools();
 		}
 
+		bridge().electronApp().initializeCustomProtocolHandler(
+			Logger.create('handleCustomProtocols'),
+		);
+		this.protocolHandler_ = bridge().electronApp().getCustomProtocolHandler();
+		this.protocolHandler_.allowReadAccessToDirectory(__dirname); // App bundle directory
+		this.protocolHandler_.allowReadAccessToDirectory(Setting.value('cacheDir'));
+		this.protocolHandler_.allowReadAccessToDirectory(Setting.value('resourceDir'));
+		// this.protocolHandler_.allowReadAccessTo(Setting.value('tempDir'));
+		// For now, this doesn't seem necessary:
+		//  this.protocolHandler_.allowReadAccessTo(Setting.value('profileDir'));
+		// If it is needed, note that they decrease the security of the protcol
+		// handler, and, as such, it may make sense to also limit permissions of
+		// allowed pages with a Content Security Policy.
+
 		PluginManager.instance().dispatch_ = this.dispatch.bind(this);
 		PluginManager.instance().setLogger(reg.logger());
 		PluginManager.instance().register(pluginClasses);

packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v5/CodeMirror.tsx

@@ -672,7 +672,7 @@ function CodeMirror(props: NoteBodyEditorProps, ref: ForwardedRef<NoteBodyEditor
 			const percent = getLineScrollPercent();
 			setEditorPercentScroll(percent);
 			options.percent = percent;
-			webviewRef.current.send('setHtml', renderedBody.html, options);
+			webviewRef.current.setHtml(renderedBody.html, options);
 		} else {
 			console.error('Trying to set HTML on an undefined webview ref');
 		}

packages/app-desktop/gui/NoteEditor/NoteBody/CodeMirror/v6/CodeMirror.tsx

@@ -289,7 +289,7 @@ const CodeMirror = (props: NoteBodyEditorProps, ref: ForwardedRef<NoteBodyEditor
 			const percent = getLineScrollPercent();
 			setEditorPercentScroll(percent);
 			options.percent = percent;
-			webviewRef.current.send('setHtml', renderedBody.html, options);
+			webviewRef.current.setHtml(renderedBody.html, options);
 		} else {
 			console.error('Trying to set HTML on an undefined webview ref');
 		}

packages/app-desktop/gui/NoteEditor/NoteEditor.tsx

@@ -138,7 +138,7 @@ function NoteEditor(props: NoteEditorProps) {
 		const theme = themeStyle(options.themeId ? options.themeId : props.themeId);
 
 		const markupToHtml = markupLanguageUtils.newMarkupToHtml({}, {
-			resourceBaseUrl: `file://${Setting.value('resourceDir')}/`,
+			resourceBaseUrl: `joplin-content://note-viewer/${Setting.value('resourceDir')}/`,
 			customCss: props.customCss,
 		});
 

packages/app-desktop/gui/NoteEditor/utils/useMarkupToHtml.ts

@@ -22,7 +22,7 @@ export default function useMarkupToHtml(deps: HookDependencies) {
 
 	const markupToHtml = useMemo(() => {
 		return markupLanguageUtils.newMarkupToHtml(plugins, {
-			resourceBaseUrl: `file://${Setting.value('resourceDir')}/`,
+			resourceBaseUrl: `joplin-content://note-viewer/${Setting.value('resourceDir')}/`,
 			customCss: customCss || '',
 		});
 	}, [plugins, customCss]);

packages/app-desktop/gui/NoteRevisionViewer.tsx

@@ -140,7 +140,7 @@ class NoteRevisionViewerComponent extends React.PureComponent<Props, State> {
 		const theme = themeStyle(this.props.themeId);
 
 		const markupToHtml = markupLanguageUtils.newMarkupToHtml({}, {
-			resourceBaseUrl: `file://${Setting.value('resourceDir')}/`,
+			resourceBaseUrl: `joplin-content://note-viewer/${Setting.value('resourceDir')}/`,
 			customCss: this.props.customCss ? this.props.customCss : '',
 		});
 
@@ -150,7 +150,7 @@ class NoteRevisionViewerComponent extends React.PureComponent<Props, State> {
 			postMessageSyntax: 'ipcProxySendToHost',
 		});
 
-		this.viewerRef_.current.send('setHtml', result.html, {
+		this.viewerRef_.current.setHtml(result.html, {
 			// cssFiles: result.cssFiles,
 			pluginAssets: result.pluginAssets,
 		});

packages/app-desktop/gui/NoteTextViewer.tsx

@@ -1,6 +1,7 @@
 import PostMessageService, { MessageResponse, ResponderComponentType } from '@joplin/lib/services/PostMessageService';
 import * as React from 'react';
 import { reg } from '@joplin/lib/registry';
+import bridge from '../services/bridge';
 import { focus } from '@joplin/lib/utils/focusHandler';
 
 interface Props {
@@ -14,6 +15,12 @@ interface Props {
 	themeId: number;
 }
 
+type RemovePluginAssetsCallback = ()=> void;
+
+interface SetHtmlOptions {
+	pluginAssets: { path: string }[];
+}
+
 // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 export default class NoteTextViewerComponent extends React.Component<Props, any> {
 
@@ -23,6 +30,7 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
 	private webviewRef_: any;
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 	private webviewListeners_: any = null;
+	private removePluginAssetsCallback_: RemovePluginAssetsCallback|null = null;
 
 	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
 	public constructor(props: any) {
@@ -64,8 +72,8 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
 		this.webview_domReady({});
 	}
 
-	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
+	private webview_message(event: MessageEvent) {
-	private webview_message(event: any) {
+		if (event.source !== this.webviewRef_.current?.contentWindow) return;
 		if (!event.data || event.data.target !== 'main') return;
 
 		const callName = event.data.name;
@@ -100,7 +108,7 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
 			wv.addEventListener(n, fn);
 		}
 
-		this.webviewRef_.current.contentWindow.addEventListener('message', this.webview_message);
+		window.addEventListener('message', this.webview_message);
 	}
 
 	public destroyWebview() {
@@ -113,17 +121,12 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
 			wv.removeEventListener(n, fn);
 		}
 
-		try {
+		window.removeEventListener('message', this.webview_message);
-			// It seems this can throw a cross-origin error in a way that is hard to replicate so just wrap
-			// it in try/catch since it's not critical.
-			// https://github.com/laurent22/joplin/issues/3835
-			this.webviewRef_.current.contentWindow.removeEventListener('message', this.webview_message);
-		} catch (error) {
-			reg.logger().warn('Error destroying note viewer', error);
-		}
 
 		this.initialized_ = false;
 		this.domReady_ = false;
+
+		this.removePluginAssetsCallback_?.();
 	}
 
 	public focus() {
@@ -163,6 +166,7 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
 			win.postMessage({ target: 'webview', name: 'focus', data: {} }, '*');
 		}
 
+		// External code should use .setHtml (rather than send('setHtml', ...))
 		if (channel === 'setHtml') {
 			win.postMessage({ target: 'webview', name: 'setHtml', data: { html: arg0, options: arg1 } }, '*');
 		}
@@ -180,12 +184,48 @@ export default class NoteTextViewerComponent extends React.Component<Props, any>
 		}
 	}
 
+	// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
+	public setHtml(html: string, options: SetHtmlOptions) {
+		// Grant & remove asset access.
+		if (options.pluginAssets) {
+			this.removePluginAssetsCallback_?.();
+
+			const protocolHandler = bridge().electronApp().getCustomProtocolHandler();
+
+			const pluginAssetPaths: string[] = options.pluginAssets.map((asset) => asset.path);
+			const assetAccesses = pluginAssetPaths.map(
+				path => protocolHandler.allowReadAccessToFile(path),
+			);
+
+			this.removePluginAssetsCallback_ = () => {
+				for (const accessControl of assetAccesses) {
+					accessControl.remove();
+				}
+
+				this.removePluginAssetsCallback_ = null;
+			};
+		}
+
+		this.send('setHtml', html, options);
+	}
+
 	// ----------------------------------------------------------------
 	// Wrap WebView functions (END)
 	// ----------------------------------------------------------------
 
 	public render() {
 		const viewerStyle = { border: 'none', ...this.props.viewerStyle };
-		return <iframe className="noteTextViewer" ref={this.webviewRef_} style={viewerStyle} src="gui/note-viewer/index.html"></iframe>;
+
+		// allow=fullscreen: Required to allow the user to fullscreen videos.
+		return (
+			<iframe
+				className="noteTextViewer"
+				ref={this.webviewRef_}
+				style={viewerStyle}
+				allow='fullscreen=* autoplay=* local-fonts=* encrypted-media=*'
+				allowFullScreen={true}
+				src={`joplin-content://note-viewer/${__dirname}/note-viewer/index.html`}
+			></iframe>
+		);
 	}
 }

packages/app-desktop/gui/note-viewer/index.html

@@ -51,7 +51,7 @@
 	// This is function used internally to send message from the webview to
 	// the host.
 	const ipcProxySendToHost = (methodName, arg) => {
-		window.postMessage({ target: 'main', name: methodName, args: [ arg ] }, '*');
+		window.parent.postMessage({ target: 'main', name: methodName, args: [ arg ] }, '*');
 	}
 
 	const webviewApiPromises_ = {};
@@ -182,14 +182,22 @@
 
 				let element = null;
 
+				// Needed on Windows:
+				//   C:/Path/Here
+				// is interpreted as a file path, even without a starting file://.
+				let src = encodedPath;
+				if (src.match(/^[/]/) || src.match(/^[^:/\\]+[:][\\/]/)) {
+					src = `joplin-content://note-viewer/${src}`;
+				}
+
 				if (asset.mime === 'application/javascript') {
 					element = document.createElement('script');
-					element.src = encodedPath;
+					element.src = src;
 					pluginAssetsContainer.appendChild(element);
 				} else if (asset.mime === 'text/css') {
 					element = document.createElement('link');
 					element.rel = 'stylesheet';
-					element.href = encodedPath
+					element.href = src;
 					pluginAssetsContainer.appendChild(element);
 				}
 

packages/app-desktop/jest.config.js

@@ -135,7 +135,7 @@ module.exports = {
 
 	// The glob patterns Jest uses to detect test files
 	testMatch: [
-		'**/*.test.js',
+		'**/*.test.(ts|tsx)',
 	],
 
 	// The regexp pattern or array of patterns that Jest uses to detect test files
@@ -154,7 +154,9 @@ module.exports = {
 	// timers: "real",
 
 	// A map from regular expressions to paths to transformers
-	// transform: undefined,
+	transform: {
+		'\\.(ts|tsx)$': 'ts-jest',
+	},
 
 	// An array of regexp pattern strings that are matched against all modules before the module loader will automatically return a mock for them
 	// unmockedModulePathPatterns: undefined,

packages/app-desktop/main.js

@@ -11,6 +11,7 @@ const envFromArgs = require('@joplin/lib/envFromArgs');
 const packageInfo = require('./packageInfo.js');
 const { isCallbackUrl } = require('@joplin/lib/callbackUrlUtils');
 const determineBaseAppDirs = require('@joplin/lib/determineBaseAppDirs').default;
+const registerCustomProtocols = require('./utils/customProtocols/registerCustomProtocols.js').default;
 
 // Electron takes the application name from package.json `name` and
 // displays this in the tray icon toolip and message box titles, however in
@@ -60,6 +61,7 @@ if (pathExistsSync(settingsPath)) {
 }
 
 electronApp.setAsDefaultProtocolClient('joplin');
+void registerCustomProtocols();
 
 const initialCallbackUrl = process.argv.find((arg) => isCallbackUrl(arg));
 

packages/app-desktop/package.json

@@ -140,6 +140,7 @@
     "js-sha512": "0.9.0",
     "nan": "2.19.0",
     "react-test-renderer": "18.2.0",
+    "ts-jest": "29.1.1",
     "ts-node": "10.9.2",
     "typescript": "5.2.2"
   },

packages/app-desktop/utils/customProtocols/constants.ts

@@ -0,0 +1,3 @@
+
+// eslint-disable-next-line import/prefer-default-export
+export const contentProtocolName = 'joplin-content';

packages/app-desktop/utils/customProtocols/handleCustomProtocols.test.ts

@@ -0,0 +1,131 @@
+/** @jest-environment node */
+
+type ProtocolHandler = (request: Request)=> Promise<Response>;
+const customProtocols: Map<string, ProtocolHandler> = new Map();
+
+const handleProtocolMock = jest.fn((scheme: string, handler: ProtocolHandler) => {
+	customProtocols.set(scheme, handler);
+});
+const fetchMock = jest.fn(async url => new Response(`Mock response to ${url}`));
+
+jest.doMock('electron', () => {
+	return {
+		net: {
+			fetch: fetchMock,
+		},
+		protocol: {
+			handle: handleProtocolMock,
+		},
+	};
+});
+
+import Logger from '@joplin/utils/Logger';
+import handleCustomProtocols from './handleCustomProtocols';
+import { supportDir } from '@joplin/lib/testing/test-utils';
+import { join } from 'path';
+import { stat } from 'fs-extra';
+import { toForwardSlashes } from '@joplin/utils/path';
+
+const setUpProtocolHandler = () => {
+	const logger = Logger.create('test-logger');
+	const protocolHandler = handleCustomProtocols(logger);
+
+	expect(handleProtocolMock).toHaveBeenCalledTimes(1);
+
+	// Should have registered the protocol.
+	const lastCallArgs = handleProtocolMock.mock.lastCall;
+	expect(lastCallArgs[0]).toBe('joplin-content');
+
+	// Extract the request listener so that it can be called by our tests.
+	const onRequestListener = lastCallArgs[1];
+
+	return { protocolHandler, onRequestListener };
+};
+
+const expectPathToBeBlocked = async (onRequestListener: ProtocolHandler, filePath: string) => {
+	const url = `joplin-content://note-viewer/${filePath}`;
+
+	await expect(
+		async () => await onRequestListener(new Request(url)),
+	).rejects.toThrowError('Read access not granted for URL');
+};
+
+const expectPathToBeUnblocked = async (onRequestListener: ProtocolHandler, filePath: string) => {
+	const handleRequestResult = await onRequestListener(new Request(`joplin-content://note-viewer/${filePath}`));
+	expect(handleRequestResult.body).toBeTruthy();
+};
+
+
+describe('handleCustomProtocols', () => {
+	beforeEach(() => {
+		// Reset mocks between tests to ensure a clean testing environment.
+		customProtocols.clear();
+		handleProtocolMock.mockClear();
+		fetchMock.mockClear();
+	});
+
+	test('should only allow access to files in allowed directories', async () => {
+		const { protocolHandler, onRequestListener } = setUpProtocolHandler();
+
+		await expectPathToBeBlocked(onRequestListener, '/test/path');
+		await expectPathToBeBlocked(onRequestListener, '/');
+
+		protocolHandler.allowReadAccessToDirectory('/test/path/');
+		await expectPathToBeUnblocked(onRequestListener, '/test/path');
+		await expectPathToBeUnblocked(onRequestListener, '/test/path/a.txt');
+		await expectPathToBeUnblocked(onRequestListener, '/test/path/b.txt');
+
+		await expectPathToBeBlocked(onRequestListener, '/');
+		await expectPathToBeBlocked(onRequestListener, '/test/path2');
+		await expectPathToBeBlocked(onRequestListener, '/test/path/../a.txt');
+
+		protocolHandler.allowReadAccessToDirectory('/another/path/here');
+
+		await expectPathToBeBlocked(onRequestListener, '/another/path/here2');
+		await expectPathToBeUnblocked(onRequestListener, '/another/path/here');
+		await expectPathToBeUnblocked(onRequestListener, '/another/path/here/2');
+	});
+
+	test('should be possible to allow and remove read access for a file', async () => {
+		const { protocolHandler, onRequestListener } = setUpProtocolHandler();
+		await expectPathToBeBlocked(onRequestListener, '/test/path/a.txt');
+
+		const handle1 = protocolHandler.allowReadAccessToFile('/test/path/a.txt');
+		await expectPathToBeUnblocked(onRequestListener, '/test/path/a.txt');
+		const handle2 = protocolHandler.allowReadAccessToFile('/test/path/a.txt');
+		await expectPathToBeUnblocked(onRequestListener, '/test/path/a.txt');
+		handle1.remove();
+		await expectPathToBeUnblocked(onRequestListener, '/test/path/a.txt');
+		handle2.remove();
+
+		await expectPathToBeBlocked(onRequestListener, '/test/path/a.txt');
+	});
+
+	test('should allow requesting part of a file', async () => {
+		const { protocolHandler, onRequestListener } = setUpProtocolHandler();
+
+		protocolHandler.allowReadAccessToDirectory(`${supportDir}/`);
+		const targetFilePath = join(supportDir, 'photo.jpg');
+		const targetUrl = `joplin-content://note-viewer/${toForwardSlashes(targetFilePath)}`;
+
+		// Should return correct headers for an in-range response,
+		let response = await onRequestListener(new Request(
+			targetUrl,
+			{ headers: { 'Range': 'bytes=10-20' } },
+		));
+
+		expect(response.status).toBe(206); // Partial content
+		expect(response.headers.get('Accept-Ranges')).toBe('bytes');
+		expect(response.headers.get('Content-Type')).toBe('image/jpeg');
+		expect(response.headers.get('Content-Length')).toBe('11');
+		const targetStats = await stat(targetFilePath);
+		expect(response.headers.get('Content-Range')).toBe(`bytes 10-20/${targetStats.size}`);
+
+		// Should return correct headers for an out-of-range response,
+		response = await onRequestListener(new Request(
+			targetUrl,
+			{ headers: { 'Range': 'bytes=99999999999999-999999999999990' } },
+		));
+		expect(response.status).toBe(416); // Out of range
+	});
+});

packages/app-desktop/utils/customProtocols/handleCustomProtocols.ts

@@ -0,0 +1,157 @@
+import { net, protocol } from 'electron';
+import { dirname, resolve, normalize } from 'path';
+import { fileURLToPath, pathToFileURL } from 'url';
+import { contentProtocolName } from './constants';
+import resolvePathWithinDir from '@joplin/lib/utils/resolvePathWithinDir';
+import { LoggerWrapper } from '@joplin/utils/Logger';
+import * as fs from 'fs-extra';
+import { createReadStream } from 'fs';
+import { Readable } from 'stream';
+import { fromFilename } from '@joplin/lib/mime-utils';
+
+export interface CustomProtocolHandler {
+	allowReadAccessToDirectory(path: string): void;
+	allowReadAccessToFile(path: string): { remove(): void };
+}
+
+// Allows seeking videos.
+// See https://github.com/electron/electron/issues/38749 for why this is necessary.
+const handleRangeRequest = async (request: Request, targetPath: string) => {
+	const makeUnsupportedRangeResponse = () => {
+		return new Response('unsupported range', {
+			status: 416, // Range Not Satisfiable
+		});
+	};
+
+	const rangeHeader = request.headers.get('Range');
+	if (!rangeHeader.startsWith('bytes=')) {
+		return makeUnsupportedRangeResponse();
+	}
+
+	const stat = await fs.stat(targetPath);
+	// Ranges are requested using one of the following formats
+	//  bytes=1234-5679
+	//  bytes=-5678
+	//  bytes=1234-
+	// See https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Range
+	const startByte = Number(rangeHeader.match(/(\d+)-/)?.[1] || '0');
+	const endByte = Number(rangeHeader.match(/-(\d+)/)?.[1] || `${stat.size - 1}`);
+
+	if (endByte > stat.size || startByte < 0) {
+		return makeUnsupportedRangeResponse();
+	}
+
+	// Note: end is inclusive.
+	const resultStream = Readable.toWeb(createReadStream(targetPath, { start: startByte, end: endByte }));
+
+	// See the HTTP range requests guide: https://developer.mozilla.org/en-US/docs/Web/HTTP/Range_requests
+	const headers = new Headers([
+		['Accept-Ranges', 'bytes'],
+		['Content-Type', fromFilename(targetPath)],
+		['Content-Length', `${endByte + 1 - startByte}`],
+		['Content-Range', `bytes ${startByte}-${endByte}/${stat.size}`],
+	]);
+
+	return new Response(
+		// This cast is necessary -- .toWeb produces a different type
+		// from the global ReadableStream.
+		resultStream as ReadableStream,
+		{ headers, status: 206 },
+	);
+};
+
+// Creating a custom protocol allows us to isolate iframes by giving them
+// different domain names from the main Joplin app.
+//
+// For example, an iframe with url joplin-content://note-viewer/path/to/iframe.html will run
+// in a different process from a parent frame with url file://path/to/iframe.html.
+//
+// See note_viewer_isolation.md for why this is important.
+//
+// TODO: Use Logger.create (doesn't work for now because Logger is only initialized
+// in the main process.)
+const handleCustomProtocols = (logger: LoggerWrapper): CustomProtocolHandler => {
+	const readableDirectories: string[] = [];
+	const readableFiles = new Map<string, number>();
+
+	// See also the protocol.handle example: https://www.electronjs.org/docs/latest/api/protocol#protocolhandlescheme-handler
+	protocol.handle(contentProtocolName, async request => {
+		const url = new URL(request.url);
+		const host = url.host;
+
+		let pathname = normalize(fileURLToPath(`file://${url.pathname}`));
+
+		// See https://security.stackexchange.com/a/123723
+		if (pathname.startsWith('..')) {
+			throw new Error(`Invalid URL (not absolute), ${request.url}`);
+		}
+
+		pathname = resolve(appBundleDirectory, pathname);
+
+		const allowedHosts = ['note-viewer'];
+
+		let canRead = false;
+		if (allowedHosts.includes(host)) {
+			if (readableFiles.has(pathname)) {
+				canRead = true;
+			} else {
+				for (const readableDirectory of readableDirectories) {
+					if (resolvePathWithinDir(readableDirectory, pathname)) {
+						canRead = true;
+						break;
+					}
+				}
+			}
+		} else {
+			throw new Error(`Invalid URL ${request.url}`);
+		}
+
+		if (!canRead) {
+			throw new Error(`Read access not granted for URL ${request.url}`);
+		}
+
+		const asFileUrl = pathToFileURL(pathname).toString();
+		logger.debug('protocol handler: Fetch file URL', asFileUrl);
+
+		const rangeHeader = request.headers.get('Range');
+		if (!rangeHeader) {
+			const response = await net.fetch(asFileUrl);
+			return response;
+		} else {
+			return handleRangeRequest(request, pathname);
+		}
+	});
+
+	const appBundleDirectory = dirname(dirname(__dirname));
+	return {
+		allowReadAccessToDirectory: (path: string) => {
+			path = resolve(appBundleDirectory, path);
+			logger.debug('protocol handler: Allow read access to directory', path);
+
+			readableDirectories.push(path);
+		},
+		allowReadAccessToFile: (path: string) => {
+			path = resolve(appBundleDirectory, path);
+			logger.debug('protocol handler: Allow read access to file', path);
+
+			if (readableFiles.has(path)) {
+				readableFiles.set(path, readableFiles.get(path) + 1);
+			} else {
+				readableFiles.set(path, 1);
+			}
+
+			return {
+				remove: () => {
+					if ((readableFiles.get(path) ?? 0) <= 1) {
+						logger.debug('protocol handler: Remove read access to file', path);
+						readableFiles.delete(path);
+					} else {
+						readableFiles.set(path, readableFiles.get(path) - 1);
+					}
+				},
+			};
+		},
+	};
+};
+
+export default handleCustomProtocols;

packages/app-desktop/utils/customProtocols/registerCustomProtocols.ts

@@ -0,0 +1,28 @@
+
+import { protocol } from 'electron';
+import { contentProtocolName } from './constants';
+
+// This must be called before Electron's onReady event.
+// handleCustomProtocols should be called separately, after onReady.
+const registerCustomProtocols = async () => {
+	const protocolPrivileges = {
+		supportFetchAPI: true,
+
+		// Don't trigger mixed content warnings (see https://stackoverflow.com/a/75988466)
+		secure: true,
+
+		// Allows loading localStorage/sessionStorage and similar APIs
+		standard: true,
+
+		// Allows loading <video>/<audio> streaming elements
+		stream: true,
+
+		corsEnabled: true,
+		codeCache: true,
+	};
+	protocol.registerSchemesAsPrivileged([
+		{ scheme: contentProtocolName, privileges: protocolPrivileges },
+	]);
+};
+
+export default registerCustomProtocols;

packages/lib/fs-driver-base.ts

@@ -2,6 +2,7 @@ import time from './time';
 import Setting from './models/Setting';
 import { filename, fileExtension } from './path-utils';
 const md5 = require('md5');
+import resolvePathWithinDir from './utils/resolvePathWithinDir';
 import { Buffer } from 'buffer';
 
 export interface Stat {
@@ -109,9 +110,8 @@ export default class FsDriverBase {
 	// also checks that the absolute path is within baseDir, to avoid security issues.
 	// It is expected that baseDir is a safe path (not user-provided).
 	public resolveRelativePathWithinDir(baseDir: string, relativePath: string) {
-		const resolvedBaseDir = this.resolve(baseDir);
+		const resolvedPath = resolvePathWithinDir(baseDir, relativePath);
-		const resolvedPath = this.resolve(baseDir, relativePath);
+		if (!resolvedPath) throw new Error(`Resolved path for relative path "${relativePath}" is not within base directory "${baseDir}" (Was resolved to ${resolvedPath})`);
-		if (resolvedPath.indexOf(resolvedBaseDir) !== 0) throw new Error(`Resolved path for relative path "${relativePath}" is not within base directory "${baseDir}" (Was resolved to ${resolvedPath})`);
 		return resolvedPath;
 	}
 

packages/lib/services/plugins/Plugin.ts

@@ -96,7 +96,11 @@ export default class Plugin {
 		this.running_ = running;
 	}
 
-	public async dataDir(): Promise<string> {
+	public get dataDir(): string {
+		return shim.fsDriver().resolve(this.dataDir_);
+	}
+
+	public async createAndGetDataDir(): Promise<string> {
 		if (this.dataDirCreated_) return this.dataDir_;
 
 		if (!(await shim.fsDriver().exists(this.dataDir_))) {

packages/lib/services/plugins/api/JoplinPlugins.ts

@@ -68,7 +68,7 @@ export default class JoplinPlugins {
 	 * will be persisted.
 	 */
 	public async dataDir(): Promise<string> {
-		return this.plugin.dataDir();
+		return this.plugin.createAndGetDataDir();
 	}
 
 	/**

packages/lib/utils/resolvePathWithinDir.test.ts

@@ -0,0 +1,38 @@
+import resolvePathWithinDir from './resolvePathWithinDir';
+
+describe('resolvePathWithinDir', () => {
+	test('should return correct values for Unix-style paths', () => {
+		const testCases = [
+			// Absolute paths
+			{ baseDir: '/a/test/path/', path: '/a/test/path/test.txt', expected: '/a/test/path/test.txt' },
+			{ baseDir: '/a/test/path/', path: '/a/test/path/..test.txt', expected: '/a/test/path/..test.txt' },
+			{ baseDir: '/a/test/path/', path: '/a/test/path/.test.txt', expected: '/a/test/path/.test.txt' },
+			{ baseDir: '/a/test/path', path: '/a/test/path/.test.txt', expected: '/a/test/path/.test.txt' },
+			{ baseDir: '/a/test/path', path: '/a/test/path', expected: '/a/test/path' },
+			{ baseDir: '/a/test/path', path: '/a/', expected: null },
+			{ baseDir: '/a/test/path', path: '/a/test/', expected: null },
+			{ baseDir: '/a/test/path', path: '/a/test/pa', expected: null },
+			{ baseDir: '/a/test/path', path: '/a/test/path2', expected: null },
+			{ baseDir: '/a/test/path', path: '/a/test/path\\//subdir', expected: null },
+
+			// Relative paths
+			{ baseDir: '/a/test/path', path: './test', expected: '/a/test/path/test' },
+			{ baseDir: '/a/test/path', path: '../path/test/2', expected: '/a/test/path/test/2' },
+			{ baseDir: '/a/test/path', path: '../path/test/../../../2', expected: null },
+			{ baseDir: '/a/test/path', path: '../test', expected: null },
+		];
+
+		for (const testCase of testCases) {
+			expect(resolvePathWithinDir(testCase.baseDir, testCase.path, false)).toBe(testCase.expected);
+		}
+	});
+
+	test('should return correct values for Windows-style paths', () => {
+		expect(resolvePathWithinDir('C:\\a\\test\\path', 'C:\\a\\test\\path\\2', true)).toBe('C:\\a\\test\\path\\2');
+		expect(resolvePathWithinDir('C:\\\\a\\test\\path', '.\\path\\2', true)).toBe('C:\\a\\test\\path\\path\\2');
+		expect(resolvePathWithinDir('C:\\a\\test\\path', '..\\path\\2', true)).toBe('C:\\a\\test\\path\\2');
+		expect(resolvePathWithinDir('C:\\a\\test\\path', '..\\2', true)).toBe(null);
+		expect(resolvePathWithinDir('D:\\a\\test\\path', 'C:\\a\\test\\path\\2', true)).toBe(null);
+		expect(resolvePathWithinDir('\\a\\test\\path', 'D:\\a\\test\\path\\2', true)).toBe(null);
+	});
+});

packages/lib/utils/resolvePathWithinDir.ts

@@ -0,0 +1,45 @@
+
+import * as path from 'path';
+
+// Returns `null` if `relativePath` is not within `baseDir` and `relativePath`
+// resolved to an absolute path otherwise.
+//
+// `relativePath` can be either relative or absolute.
+// If relative, it is assumed to be relative to `baseDir`.
+//
+// It is expected that baseDir is a safe path (not user-provided).
+const resolvePathWithinDir = (
+	baseDir: string, relativePath: string,
+
+	// For testing
+	forceWin32Paths?: boolean,
+) => {
+	let pathModule = path;
+	if (forceWin32Paths === true) {
+		pathModule = path.win32;
+	}
+
+	let resolvedBaseDir = pathModule.resolve(baseDir);
+	const resolvedPath = pathModule.resolve(baseDir, relativePath);
+
+	// Handles the case where resolvedBaseDir doesn't end with a
+	// path separator. For example, if
+	//   resolvedBaseDir="/foo/bar"
+	// then we could have
+	//   resolvedPath="/foo/bar2"
+	// which is not within the "/foo/bar" directory.
+	//
+	// We can't do this if the two paths are already equal as (as this would cause
+	// resolvedPath to no longer start with resolvedBaseDir).
+	if (!resolvedBaseDir.endsWith(pathModule.sep) && resolvedBaseDir !== resolvedPath) {
+		resolvedBaseDir += pathModule.sep;
+	}
+
+	if (!resolvedPath.startsWith(resolvedBaseDir)) {
+		return null;
+	}
+
+	return resolvedPath;
+};
+
+export default resolvePathWithinDir;

readme/dev/spec/note_viewer_isolation.md

@@ -0,0 +1,48 @@
+# Note viewer isolation
+
+The desktop application's note viewer runs in an `iframe` with a different protocol from the main application's top-level frame.
+
+## Rationale
+
+Running the note viewer from a different protocol and/or domain moves the `iframe`'s content to a separate process. This has at least two benefits:
+1. Reduces the impact of bugs in Joplin's HTML sanitizer.
+2. Improves performance when rendering large notes (see [issue #10424](https://github.com/laurent22/joplin/issues/10424)).
+
+## Why does a different protocol improve performance/security?
+
+Using a different domain and/or protocol for the note viewer does the following:
+1. [enables Chromium's site isolation](https://www.chromium.org/developers/design-documents/site-isolation/) and
+2. [prevents the note viewer from accessing the top-level context through `window.top`](https://developer.mozilla.org/en-US/docs/Web/Security/Same-origin_policy#cross-origin_script_api_access).
+
+## The protocol
+
+We use Electron's [protocol.handle](https://www.electronjs.org/docs/latest/api/protocol#protocolhandlescheme-handler) to register the `joplin-content://` protocol.
+
+We use a `file://` URL to load the base electron application.
+
+We use a `joplin-content://note-viewer/` URL to load the note viewer and the resources it contains.
+
+For compatibility with renderer plugins that use absolute resource paths `joplin-content://note-viewer/` fetches files relative to the `/` directory.
+
+Here's an example:
+- The note viewer loads with the URL `joplin-content://note-viewer/tmp/.mount_JoplinA7E0A/path/to/the/note/viewer/here.html`
+- The note renders and includes a resource with the path `/home/user/.config/joplin-desktop/path/here.css`
+- `/home/user/.config/joplin-desktop/path/here.css` is converted to `joplin-content://note-viewer/home/user/.config/joplin-desktop/path/here.css` by Electron.
+- Joplin checks to make sure the `joplin-content://` protocol has access to `/home/user/.config/joplin-desktop/path/here.css`. If it does, it fetches and returns the file.
+
+
+## `joplin-content://` only has access to specific directories
+
+When `handleCustomProtocols` creates a handler for the `joplin-content://` protocol, it returns an object that allows certain directories to be marked as readable.
+
+By default, the list of readable directories includes:
+- The app bundle
+- Data directories for each of the enabled plugins
+- The resource directory
+- The profile directory
+
+
+## Why not the [`sandbox`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#sandbox) property?
+
+Enabling `sandbox` disables Electron's PDF viewer ([related HTML spec change](https://github.com/whatwg/html/pull/6946)) and prevents content within the note viewer from displaying/requesting resources.
+

yarn.lock

@@ -6824,6 +6824,7 @@ __metadata:
     taboverride: 4.0.3
     tesseract.js: 5.0.5
     tinymce: 5.10.6
+    ts-jest: 29.1.1
     ts-node: 10.9.2
     typescript: 5.2.2
   languageName: unknown