diff --git a/packages/app-desktop/app.ts b/packages/app-desktop/app.ts index 311095269..0a26f8aef 100644 --- a/packages/app-desktop/app.ts +++ b/packages/app-desktop/app.ts @@ -127,6 +127,12 @@ class Application extends BaseApplication { bridge().setLocale(Setting.value('locale')); } + if (action.type === 'SETTING_UPDATE_ONE' && action.key === 'renderer.fileUrls' || action.type === 'SETTING_UPDATE_ALL') { + bridge().electronApp().getCustomProtocolHandler().setMediaAccessEnabled( + Setting.value('renderer.fileUrls'), + ); + } + if (action.type === 'SETTING_UPDATE_ONE' && action.key === 'showTrayIcon' || action.type === 'SETTING_UPDATE_ALL') { this.updateTray(); } diff --git a/packages/app-desktop/gui/NoteTextViewer.tsx b/packages/app-desktop/gui/NoteTextViewer.tsx index debb4451e..acc789227 100644 --- a/packages/app-desktop/gui/NoteTextViewer.tsx +++ b/packages/app-desktop/gui/NoteTextViewer.tsx @@ -29,6 +29,7 @@ export default class NoteTextViewerComponent extends React.Component private webviewRef_: React.RefObject; // 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 @@ -110,7 +111,7 @@ export default class NoteTextViewerComponent extends React.Component window.addEventListener('message', this.webview_message); } - public destroyWebview() { + private destroyWebview() { const wv = this.webviewRef_.current; if (!wv || !this.initialized_) return; @@ -194,14 +195,13 @@ export default class NoteTextViewerComponent extends React.Component } } - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied public setHtml(html: string, options: SetHtmlOptions) { + const protocolHandler = bridge().electronApp().getCustomProtocolHandler(); + // 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), @@ -216,7 +216,10 @@ export default class NoteTextViewerComponent extends React.Component }; } - this.send('setHtml', html, options); + this.send('setHtml', html, { + ...options, + mediaAccessKey: protocolHandler.getMediaAccessKey(), + }); } // ---------------------------------------------------------------- diff --git a/packages/app-desktop/gui/note-viewer/index.html b/packages/app-desktop/gui/note-viewer/index.html index 55531e1fd..6f016f7df 100644 --- a/packages/app-desktop/gui/note-viewer/index.html +++ b/packages/app-desktop/gui/note-viewer/index.html @@ -377,6 +377,20 @@ contentElement.scrollTop = scrollTop; } + const rewriteFileUrls = (accessKey) => { + if (!accessKey) return; + + // To allow accessing local files from the viewer's non-file URL, file:// URLs are re-written + // to joplin-content:// URLs: + const mediaElements = document.querySelectorAll('video[src], audio[src], source[src], img[src]'); + for (const element of mediaElements) { + if (element.src?.startsWith('file:')) { + const newUrl = element.src.replace(/^file:\/\//, 'joplin-content://file-media/'); + element.src = `${newUrl}?access-key=${accessKey}`; + } + } + }; + ipc.setHtml = (event) => { const html = event.html; @@ -388,6 +402,10 @@ contentElement.innerHTML = html; + if (html.includes('file://')) { + rewriteFileUrls(event.options.mediaAccessKey); + } + scrollmap.create(event.options.markupLineCount); if (typeof event.options.percent !== 'number') { restorePercentScroll(); // First, a quick treatment is applied. diff --git a/packages/app-desktop/utils/customProtocols/handleCustomProtocols.test.ts b/packages/app-desktop/utils/customProtocols/handleCustomProtocols.test.ts index 48d34ee36..a5a488df4 100644 --- a/packages/app-desktop/utils/customProtocols/handleCustomProtocols.test.ts +++ b/packages/app-desktop/utils/customProtocols/handleCustomProtocols.test.ts @@ -42,21 +42,29 @@ const setUpProtocolHandler = () => { return { protocolHandler, onRequestListener }; }; +interface ExpectBlockedOptions { + host?: string; +} + // Although none of the paths in this test suite point to real files, file paths must be in // a certain format on Windows to avoid invalid path exceptions. const toPlatformPath = (path: string) => process.platform === 'win32' ? `C:/${path}` : path; -const expectPathToBeBlocked = async (onRequestListener: ProtocolHandler, filePath: string) => { - const url = `joplin-content://note-viewer/${toPlatformPath(filePath)}`; - - await expect( - async () => await onRequestListener(new Request(url)), - ).rejects.toThrowError('Read access not granted for URL'); +const toAccessUrl = (path: string, { host = 'note-viewer' }: ExpectBlockedOptions = {}) => { + return `joplin-content://${host}/${toPlatformPath(path)}`; }; -const expectPathToBeUnblocked = async (onRequestListener: ProtocolHandler, filePath: string) => { +const expectPathToBeBlocked = async (onRequestListener: ProtocolHandler, filePath: string, options?: ExpectBlockedOptions) => { + const url = toAccessUrl(filePath, options); + await expect( + async () => await onRequestListener(new Request(url)), + ).rejects.toThrow(/Read access not granted for URL|Invalid or missing media access key|Media access denied/); +}; + +const expectPathToBeUnblocked = async (onRequestListener: ProtocolHandler, filePath: string, options?: ExpectBlockedOptions) => { + const url = toAccessUrl(filePath, options); const handleRequestResult = await onRequestListener( - new Request(`joplin-content://note-viewer/${toPlatformPath(filePath)}`), + new Request(url), ); expect(handleRequestResult.body).toBeTruthy(); }; @@ -107,6 +115,34 @@ describe('handleCustomProtocols', () => { await expectPathToBeBlocked(onRequestListener, '/test/path/a.txt'); }); + test('should only allow access to file-media/ URLs when given the correct access key', async () => { + const { protocolHandler, onRequestListener } = setUpProtocolHandler(); + const expectBlocked = (path: string) => { + return expectPathToBeBlocked(onRequestListener, path, { host: 'file-media' }); + }; + const expectUnblocked = (path: string) => { + return expectPathToBeUnblocked(onRequestListener, path, { host: 'file-media' }); + }; + + fetchMock.mockImplementation(async (_url: string) => { + return new Response('', { headers: { 'Content-Type': 'image/jpeg' } }); + }); + + + const testPath = join(supportDir, 'photo.jpg'); + await expectBlocked(testPath); + await expectBlocked(`${testPath}?access-key=wrongKey`); + await expectBlocked(`${testPath}?access-key=false`); + + protocolHandler.setMediaAccessEnabled(true); + const key = protocolHandler.getMediaAccessKey(); + await expectUnblocked(`${testPath}?access-key=${key}`); + await expectBlocked(`${testPath}?access-key=null`); + protocolHandler.setMediaAccessEnabled(false); + + await expectBlocked(`${testPath}?access-key=${key}`); + }); + test('should allow requesting part of a file', async () => { const { protocolHandler, onRequestListener } = setUpProtocolHandler(); diff --git a/packages/app-desktop/utils/customProtocols/handleCustomProtocols.ts b/packages/app-desktop/utils/customProtocols/handleCustomProtocols.ts index 9cd80182f..354be211c 100644 --- a/packages/app-desktop/utils/customProtocols/handleCustomProtocols.ts +++ b/packages/app-desktop/utils/customProtocols/handleCustomProtocols.ts @@ -7,10 +7,20 @@ import { LoggerWrapper } from '@joplin/utils/Logger'; import * as fs from 'fs-extra'; import { createReadStream } from 'fs'; import { fromFilename } from '@joplin/lib/mime-utils'; +import { createSecureRandom } from '@joplin/lib/uuid'; + +export interface AccessController { + remove(): void; +} export interface CustomProtocolHandler { + // note-viewer/ URLs allowReadAccessToDirectory(path: string): void; - allowReadAccessToFile(path: string): { remove(): void }; + allowReadAccessToFile(path: string): AccessController; + + // file-media/ URLs + setMediaAccessEnabled(enabled: boolean): void; + getMediaAccessKey(): string; } @@ -130,8 +140,11 @@ const handleCustomProtocols = (logger: LoggerWrapper): CustomProtocolHandler => debug: () => {}, }; + // Allow-listed files/directories for joplin-content://note-viewer/ const readableDirectories: string[] = []; const readableFiles = new Map(); + // Access for joplin-content://file-media/ + let mediaAccessKey: string|false = false; // See also the protocol.handle example: https://www.electronjs.org/docs/latest/api/protocol#protocolhandlescheme-handler protocol.handle(contentProtocolName, async request => { @@ -147,10 +160,9 @@ const handleCustomProtocols = (logger: LoggerWrapper): CustomProtocolHandler => pathname = resolve(appBundleDirectory, pathname); - const allowedHosts = ['note-viewer']; - let canRead = false; - if (allowedHosts.includes(host)) { + let mediaOnly = true; + if (host === 'note-viewer') { if (readableFiles.has(pathname)) { canRead = true; } else { @@ -161,6 +173,20 @@ const handleCustomProtocols = (logger: LoggerWrapper): CustomProtocolHandler => } } } + + mediaOnly = false; + } else if (host === 'file-media') { + if (!mediaAccessKey) { + throw new Error('Media access denied. This must be enabled with .setMediaAccessEnabled'); + } + + canRead = true; + mediaOnly = true; + + const accessKey = url.searchParams.get('access-key'); + if (accessKey !== mediaAccessKey) { + throw new Error(`Invalid or missing media access key (was ${accessKey}). An allow-listed ?access-key= parameter must be provided.`); + } } else { throw new Error(`Invalid URL ${request.url}`); } @@ -173,12 +199,26 @@ const handleCustomProtocols = (logger: LoggerWrapper): CustomProtocolHandler => logger.debug('protocol handler: Fetch file URL', asFileUrl); const rangeHeader = request.headers.get('Range'); + let response; if (!rangeHeader) { - const response = await net.fetch(asFileUrl); - return response; + response = await net.fetch(asFileUrl); } else { - return handleRangeRequest(request, pathname); + response = await handleRangeRequest(request, pathname); } + + if (mediaOnly) { + // Tells the browser to avoid MIME confusion attacks. See + // https://blog.mozilla.org/security/2016/08/26/mitigating-mime-confusion-attacks-in-firefox/ + response.headers.set('X-Content-Type-Options', 'nosniff'); + + // This is an extra check to prevent loading text/html and arbitrary non-media content from the URL. + const contentType = response.headers.get('Content-Type'); + if (!contentType || !contentType.match(/^(image|video|audio)\//)) { + throw new Error(`Attempted to access non-media file from ${request.url}, which is media-only. Content type was ${contentType}.`); + } + } + + return response; }); const appBundleDirectory = dirname(dirname(__dirname)); @@ -210,6 +250,18 @@ const handleCustomProtocols = (logger: LoggerWrapper): CustomProtocolHandler => }, }; }, + setMediaAccessEnabled: (enabled: boolean) => { + if (enabled) { + mediaAccessKey ||= createSecureRandom(); + } else { + mediaAccessKey = false; + } + }, + // Allows access to all local media files, provided a matching ?access-key= is added + // to the request URL. + getMediaAccessKey: () => { + return mediaAccessKey || null; + }, }; }; diff --git a/packages/lib/models/settings/builtInMetadata.ts b/packages/lib/models/settings/builtInMetadata.ts index 0482f4fb4..9f35bf956 100644 --- a/packages/lib/models/settings/builtInMetadata.ts +++ b/packages/lib/models/settings/builtInMetadata.ts @@ -926,6 +926,18 @@ const builtInMetadata = (Setting: typeof SettingType) => { 'markdown.plugin.insert': { storage: SettingStorage.File, isGlobal: true, value: false, type: SettingItemType.Bool, section: 'markdownPlugins', public: true, appTypes: [AppType.Mobile, AppType.Desktop], label: () => `${_('Enable ++insert++ syntax')}${wysiwygYes}` }, 'markdown.plugin.multitable': { storage: SettingStorage.File, isGlobal: true, value: false, type: SettingItemType.Bool, section: 'markdownPlugins', public: true, appTypes: [AppType.Mobile, AppType.Desktop], label: () => `${_('Enable multimarkdown table extension')}${wysiwygNo}` }, + // For now, applies only to the Markdown viewer + 'renderer.fileUrls': { + storage: SettingStorage.File, + isGlobal: true, + value: false, + type: SettingItemType.Bool, + section: 'markdownPlugins', + public: true, + appTypes: [AppType.Desktop], + label: () => `${_('Enable file:// URLs for images and videos')}${wysiwygYes}`, + }, + // Tray icon (called AppIndicator) doesn't work in Ubuntu // http://www.webupd8.org/2017/04/fix-appindicator-not-working-for.html // Might be fixed in Electron 18.x but no non-beta release yet. So for now diff --git a/packages/tools/cspell/dictionary4.txt b/packages/tools/cspell/dictionary4.txt index e67f10bb0..10849b8ab 100644 --- a/packages/tools/cspell/dictionary4.txt +++ b/packages/tools/cspell/dictionary4.txt @@ -132,3 +132,4 @@ Famegear rcompare tabindex Backblaze +nosniff diff --git a/readme/dev/spec/note_viewer_isolation.md b/readme/dev/spec/note_viewer_isolation.md index 742858418..ab86d9138 100644 --- a/readme/dev/spec/note_viewer_isolation.md +++ b/readme/dev/spec/note_viewer_isolation.md @@ -31,7 +31,7 @@ Here's an example: - 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 +## `joplin-content://note-viewer/` 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. @@ -41,6 +41,13 @@ By default, the list of readable directories includes: - The resource directory - The profile directory +## `joplin-content://file-media/` can only load specific file types + +To allow images and videos with `file://` URLs, Joplin maps `file://` URIs to `joplin-content://file-media/`. The `file-media/` host has the following restrictions: +- Only files with certain extensions/content-types can be loaded. + - For example, `text/html` is disallowed but `image/png` is allowed. +- A valid `?access-key=<...>` parameter must be provided with the request. + - A new access key is created for each render and old access keys are revoked. ## Why not the [`sandbox`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#sandbox) property?