diff --git a/app/src/components/contextMenu.ts b/app/src/components/contextMenu.ts index 5f0e723..0c2b4e5 100644 --- a/app/src/components/contextMenu.ts +++ b/app/src/components/contextMenu.ts @@ -18,7 +18,7 @@ export function initContextMenu( options: OutputOptions, window?: BrowserWindow, ): void { - log.debug('initContextMenu', { options, window }); + log.debug('initContextMenu'); // eslint-disable-next-line @typescript-eslint/no-unsafe-call contextMenu({ diff --git a/app/src/components/menu.ts b/app/src/components/menu.ts index 4e5f50e..cd935b7 100644 --- a/app/src/components/menu.ts +++ b/app/src/components/menu.ts @@ -45,7 +45,7 @@ export function createMenu( options: OutputOptions, mainWindow: BrowserWindow, ): void { - log.debug('createMenu', { options, mainWindow }); + log.debug('createMenu', { options }); const menuTemplate = generateMenu(options, mainWindow); injectBookmarks(menuTemplate); diff --git a/app/src/helpers/helpers.test.ts b/app/src/helpers/helpers.test.ts index 4ae396e..4f8c65e 100644 --- a/app/src/helpers/helpers.test.ts +++ b/app/src/helpers/helpers.test.ts @@ -1,9 +1,14 @@ +import { shell } from 'electron'; +jest.mock('./windowHelpers'); + import { - linkIsInternal, - getCounterValue, - removeUserAgentSpecifics, cleanupPlainText, + getCounterValue, + linkIsInternal, + openExternal, + removeUserAgentSpecifics, } from './helpers'; +import { showNavigationBlockedMessage } from './windowHelpers'; const internalUrl = 'https://medium.com/'; const internalUrlWww = 'https://www.medium.com/'; @@ -285,3 +290,59 @@ describe('cleanupPlainText', () => { expect(cleanupPlainText(' this is a test ')).toBe('this is a test'); }); }); + +describe('openExternal', () => { + const mockShellOpenExternal: jest.SpyInstance = jest.spyOn( + shell, + 'openExternal', + ); + const mockShowNavigationBlockedMessage: jest.SpyInstance = + showNavigationBlockedMessage as jest.Mock; + + beforeEach(() => { + mockShellOpenExternal.mockReset(); + mockShowNavigationBlockedMessage + .mockReset() + .mockReturnValue(Promise.resolve(undefined)); + }); + + afterAll(() => { + mockShellOpenExternal.mockRestore(); + mockShowNavigationBlockedMessage.mockRestore(); + }); + + test('https urls scheme should *not* be blocked', async () => { + await openExternal('https://whatever.foo'); + + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); + expect(mockShellOpenExternal).toHaveBeenCalled(); + }); + + test('urls with whitelisted scheme should *not* be blocked', async () => { + await openExternal('ircs://irc.libera.chat/whatever'); + + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); + expect(mockShellOpenExternal).toHaveBeenCalled(); + }); + + test('urls with non-allowlisted scheme *should* be blocked', async () => { + await openExternal('barf://whatever.foo'); + + expect(mockShowNavigationBlockedMessage).toHaveBeenCalledTimes(1); + expect(mockShellOpenExternal).not.toHaveBeenCalled(); + }); + + test('potentially-malicious urls *should* be blocked', async () => { + await openExternal('https://hello.com/wor%00ld'); + + expect(mockShowNavigationBlockedMessage).toHaveBeenCalledTimes(1); + expect(mockShellOpenExternal).not.toHaveBeenCalled(); + }); + + test('malformed urls *should* be blocked', async () => { + await openExternal('zombocom'); + + expect(mockShowNavigationBlockedMessage).toHaveBeenCalledTimes(1); + expect(mockShellOpenExternal).not.toHaveBeenCalled(); + }); +}); diff --git a/app/src/helpers/helpers.ts b/app/src/helpers/helpers.ts index 4809720..05aafc1 100644 --- a/app/src/helpers/helpers.ts +++ b/app/src/helpers/helpers.ts @@ -5,9 +5,80 @@ import * as path from 'path'; import { BrowserWindow, OpenExternalOptions, shell } from 'electron'; import * as log from '../helpers/loggingHelper'; +import { showNavigationBlockedMessage } from './windowHelpers'; export const INJECT_DIR = path.join(__dirname, '..', 'inject'); +// Taken from Firefox's. Location might vary in codebase, search for one of them, e.g. +// https://searchfox.org/mozilla-central/search?q=%22xmpp%22&path=&case=false®exp=false +const SAFE_URL_PROTOCOLS_FIREFOX = [ + 'bitcoin:', + 'ftp:', + 'ftps:', + 'geo:', + 'im:', + 'irc:', + 'ircs:', + 'magnet:', + 'mailto:', + 'matrix:', + 'mms:', + 'news:', + 'nntp:', + 'openpgp4fpr:', + 'sftp:', + 'sip:', + 'sms:', + 'smsto:', + 'ssh:', + 'tel:', + 'urn:', + 'webcal:', + 'wtai:', + 'xmpp:', +]; +const SAFE_URL_PROTOCOLS = ['http:', 'https:', ...SAFE_URL_PROTOCOLS_FIREFOX]; +const SHELL_SAFETY_FEEDBACK_STR = + 'If you believe this URL should open, you might be right, and our validation might be excessive.' + + 'Please share this error & URL at https://github.com/nativefier/nativefier/issues/1459'; + +export function isUrlShellSafe( + urlToGo: string, +): { blocked: false } | { blocked: true; reason: string } { + let url: URL; + try { + url = new URL(urlToGo.toLowerCase()); + } catch (err: unknown) { + return { + blocked: true, + reason: `URL appears malformed. ${SHELL_SAFETY_FEEDBACK_STR}`, + }; + } + + if (!SAFE_URL_PROTOCOLS.includes(url.protocol)) { + return { + blocked: true, + reason: `URL protocol is disallowed. ${SHELL_SAFETY_FEEDBACK_STR}`, + }; + } + + // https://cwe.mitre.org/data/definitions/177.html + if ( + urlToGo.includes('%00') || + urlToGo.includes('%0a') || + urlToGo.includes('%2e') || + urlToGo.includes('%2f') || + urlToGo.includes('%5c') + ) { + return { + blocked: true, + reason: `URL might be malicious. ${SHELL_SAFETY_FEEDBACK_STR}`, + }; + } + + return { blocked: false }; +} + /** * Helper to print debug messages from the main process in the browser window */ @@ -166,11 +237,28 @@ export function nativeTabsSupported(): boolean { return isOSX(); } +/** + * Open the given external protocol URL in the desktop's default manner + * (e.g. `mailto:` URLs in the user's default mail agent), with extra validation. + */ export function openExternal( url: string, options?: OpenExternalOptions, ): Promise { - log.debug('openExternal', { url, options }); + const urlShellSafety = isUrlShellSafe(url); + log.debug('openExternal', { url, options, urlShellSafety }); + if (urlShellSafety.blocked) { + return new Promise((resolve) => { + showNavigationBlockedMessage( + `Navigation blocked to ${url}\n\n${urlShellSafety.reason}`, + ) + .then(() => resolve()) + .catch((err: unknown) => { + throw err; + }); + }); + } + return shell.openExternal(url, options); } diff --git a/app/src/helpers/windowEvents.test.ts b/app/src/helpers/windowEvents.test.ts index 824cfd3..933a18d 100644 --- a/app/src/helpers/windowEvents.test.ts +++ b/app/src/helpers/windowEvents.test.ts @@ -31,7 +31,7 @@ const { onWillPreventUnload: (event: unknown) => void; } = jest.requireActual('./windowEvents'); import { - blockExternalURL, + showNavigationBlockedMessage, createAboutBlankWindow, createNewTab, } from './windowHelpers'; @@ -49,7 +49,8 @@ describe('onNewWindowHelper', () => { targetUrl: originalURL, zoom: 1.0, }; - const mockBlockExternalURL: jest.SpyInstance = blockExternalURL as jest.Mock; + const mockShowNavigationBlockedMessage: jest.SpyInstance = + showNavigationBlockedMessage as jest.Mock; const mockCreateAboutBlank: jest.SpyInstance = createAboutBlankWindow as jest.Mock; const mockCreateNewTab: jest.SpyInstance = createNewTab as jest.Mock; @@ -63,7 +64,7 @@ describe('onNewWindowHelper', () => { const setupWindow = jest.fn(); beforeEach(() => { - mockBlockExternalURL + mockShowNavigationBlockedMessage .mockReset() .mockReturnValue(Promise.resolve(undefined)); mockCreateAboutBlank.mockReset(); @@ -76,7 +77,7 @@ describe('onNewWindowHelper', () => { }); afterAll(() => { - mockBlockExternalURL.mockRestore(); + mockShowNavigationBlockedMessage.mockRestore(); mockCreateAboutBlank.mockRestore(); mockCreateNewTab.mockRestore(); mockLinkIsInternal.mockRestore(); @@ -95,7 +96,7 @@ describe('onNewWindowHelper', () => { expect(mockCreateAboutBlank).not.toHaveBeenCalled(); expect(mockCreateNewTab).not.toHaveBeenCalled(); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).not.toHaveBeenCalled(); }); @@ -113,7 +114,7 @@ describe('onNewWindowHelper', () => { expect(mockCreateAboutBlank).not.toHaveBeenCalled(); expect(mockCreateNewTab).not.toHaveBeenCalled(); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).toHaveBeenCalledTimes(1); expect(preventDefault).toHaveBeenCalledTimes(1); }); @@ -134,7 +135,7 @@ describe('onNewWindowHelper', () => { expect(mockCreateAboutBlank).not.toHaveBeenCalled(); expect(mockCreateNewTab).not.toHaveBeenCalled(); - expect(mockBlockExternalURL).toHaveBeenCalledTimes(1); + expect(mockShowNavigationBlockedMessage).toHaveBeenCalledTimes(1); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).toHaveBeenCalledTimes(1); }); @@ -150,7 +151,7 @@ describe('onNewWindowHelper', () => { expect(mockCreateAboutBlank).not.toHaveBeenCalled(); expect(mockCreateNewTab).not.toHaveBeenCalled(); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).not.toHaveBeenCalled(); }); @@ -168,7 +169,7 @@ describe('onNewWindowHelper', () => { expect(mockCreateAboutBlank).not.toHaveBeenCalled(); expect(mockCreateNewTab).not.toHaveBeenCalled(); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).toHaveBeenCalledTimes(1); expect(preventDefault).toHaveBeenCalledTimes(1); }); @@ -193,7 +194,7 @@ describe('onNewWindowHelper', () => { true, undefined, ); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).toHaveBeenCalledTimes(1); }); @@ -218,7 +219,7 @@ describe('onNewWindowHelper', () => { false, undefined, ); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).toHaveBeenCalledTimes(1); }); @@ -234,7 +235,7 @@ describe('onNewWindowHelper', () => { expect(mockCreateAboutBlank).toHaveBeenCalledTimes(1); expect(mockCreateNewTab).not.toHaveBeenCalled(); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).toHaveBeenCalledTimes(1); }); @@ -250,7 +251,7 @@ describe('onNewWindowHelper', () => { expect(mockCreateAboutBlank).toHaveBeenCalledTimes(1); expect(mockCreateNewTab).not.toHaveBeenCalled(); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).toHaveBeenCalledTimes(1); }); @@ -266,7 +267,7 @@ describe('onNewWindowHelper', () => { expect(mockCreateAboutBlank).not.toHaveBeenCalled(); expect(mockCreateNewTab).not.toHaveBeenCalled(); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).not.toHaveBeenCalled(); }); @@ -277,13 +278,14 @@ describe('onWillNavigate', () => { const internalURL = 'https://medium.com/topics/technology'; const externalURL = 'https://www.wikipedia.org/wiki/Electron'; - const mockBlockExternalURL: jest.SpyInstance = blockExternalURL as jest.Mock; + const mockShowNavigationBlockedMessage: jest.SpyInstance = + showNavigationBlockedMessage as jest.Mock; const mockLinkIsInternal: jest.SpyInstance = linkIsInternal as jest.Mock; const mockOpenExternal: jest.SpyInstance = openExternal as jest.Mock; const preventDefault = jest.fn(); beforeEach(() => { - mockBlockExternalURL + mockShowNavigationBlockedMessage .mockReset() .mockReturnValue(Promise.resolve(undefined)); mockLinkIsInternal.mockReset().mockReturnValue(false); @@ -292,7 +294,7 @@ describe('onWillNavigate', () => { }); afterAll(() => { - mockBlockExternalURL.mockRestore(); + mockShowNavigationBlockedMessage.mockRestore(); mockLinkIsInternal.mockRestore(); mockOpenExternal.mockRestore(); }); @@ -306,7 +308,7 @@ describe('onWillNavigate', () => { const event = { preventDefault }; await onWillNavigate(options, event, internalURL); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).not.toHaveBeenCalled(); }); @@ -319,12 +321,12 @@ describe('onWillNavigate', () => { const event = { preventDefault }; await onWillNavigate(options, event, externalURL); - expect(mockBlockExternalURL).not.toHaveBeenCalled(); + expect(mockShowNavigationBlockedMessage).not.toHaveBeenCalled(); expect(mockOpenExternal).toHaveBeenCalledTimes(1); expect(preventDefault).toHaveBeenCalledTimes(1); }); - test('external urls should be ignored if blockExternalUrls is true', async () => { + test('external urls should be blocked if blockExternalUrls is true', async () => { const options = { blockExternalUrls: true, targetUrl: originalURL, @@ -332,7 +334,7 @@ describe('onWillNavigate', () => { const event = { preventDefault }; await onWillNavigate(options, event, externalURL); - expect(mockBlockExternalURL).toHaveBeenCalledTimes(1); + expect(mockShowNavigationBlockedMessage).toHaveBeenCalledTimes(1); expect(mockOpenExternal).not.toHaveBeenCalled(); expect(preventDefault).toHaveBeenCalledTimes(1); }); diff --git a/app/src/helpers/windowEvents.ts b/app/src/helpers/windowEvents.ts index 59f28d3..f813985 100644 --- a/app/src/helpers/windowEvents.ts +++ b/app/src/helpers/windowEvents.ts @@ -9,12 +9,12 @@ import { import { linkIsInternal, nativeTabsSupported, openExternal } from './helpers'; import * as log from './loggingHelper'; import { - blockExternalURL, createAboutBlankWindow, createNewTab, injectCSS, sendParamsOnDidFinishLoad, setProxyRules, + showNavigationBlockedMessage, } from './windowHelpers'; import { WindowOptions } from '../../../shared/src/options/model'; @@ -86,7 +86,9 @@ export function onNewWindowHelper( preventDefault(); if (options.blockExternalUrls) { return new Promise((resolve) => { - blockExternalURL(urlToGo) + showNavigationBlockedMessage( + `Navigation to external URL blocked by options: ${urlToGo}`, + ) .then(() => resolve()) .catch((err: unknown) => { throw err; @@ -129,7 +131,7 @@ export function onWillNavigate( event: Event, urlToGo: string, ): Promise { - log.debug('onWillNavigate', { options, event, urlToGo }); + log.debug('onWillNavigate', urlToGo); if ( !linkIsInternal( options.targetUrl, @@ -141,7 +143,9 @@ export function onWillNavigate( event.preventDefault(); if (options.blockExternalUrls) { return new Promise((resolve) => { - blockExternalURL(urlToGo) + showNavigationBlockedMessage( + `Navigation to external URL blocked by options: ${urlToGo}`, + ) .then(() => resolve()) .catch((err: unknown) => { throw err; diff --git a/app/src/helpers/windowHelpers.ts b/app/src/helpers/windowHelpers.ts index f71557f..188bde7 100644 --- a/app/src/helpers/windowHelpers.ts +++ b/app/src/helpers/windowHelpers.ts @@ -23,12 +23,14 @@ export function adjustWindowZoom(adjustment: number): void { }); } -export function blockExternalURL(url: string): Promise { +export function showNavigationBlockedMessage( + message: string, +): Promise { return new Promise((resolve, reject) => { withFocusedWindow((focusedWindow) => { dialog .showMessageBox(focusedWindow, { - message: `Cannot navigate to external URL: ${url}`, + message, type: 'error', title: 'Navigation blocked', }) diff --git a/app/src/main.ts b/app/src/main.ts index 628208e..f76e081 100644 --- a/app/src/main.ts +++ b/app/src/main.ts @@ -438,14 +438,14 @@ app.on( }, ); -app.on('browser-window-blur', (event: Event, window: BrowserWindow) => { - log.debug('app.browser-window-blur', { event, window }); +app.on('browser-window-blur', () => { + log.debug('app.browser-window-blur'); }); -app.on('browser-window-created', (event: Event, window: BrowserWindow) => { - log.debug('app.browser-window-created', { event, window }); +app.on('browser-window-created', () => { + log.debug('app.browser-window-created'); }); -app.on('browser-window-focus', (event: Event, window: BrowserWindow) => { - log.debug('app.browser-window-focus', { event, window }); +app.on('browser-window-focus', () => { + log.debug('app.browser-window-focus'); }); diff --git a/app/src/mocks/electron.ts b/app/src/mocks/electron.ts index fd7b62c..7f6aa17 100644 --- a/app/src/mocks/electron.ts +++ b/app/src/mocks/electron.ts @@ -144,10 +144,17 @@ class MockWebRequest { class InternalEmitter extends EventEmitter {} +const mockShell = { + openExternal(url: string, options?: unknown): Promise { + return new Promise((resolve) => resolve()); + }, +}; + export { MockDialog as dialog, MockBrowserWindow as BrowserWindow, MockSession as Session, MockWebContents as WebContents, MockWebRequest as WebRequest, + mockShell as shell, }; diff --git a/package.json b/package.json index f61b63e..4cfe440 100644 --- a/package.json +++ b/package.json @@ -52,6 +52,7 @@ "test:noplaywright": "jest --testPathIgnorePatterns=playwright", "test:unit": "jest", "test:watch": "echo 'Remember to run npm run build:watch for the test watcher to work!' && jest --watchAll --collectCoverage=false", + "test:watch:unit": "echo 'Remember to run npm run build:watch for the test watcher to work!' && jest --watchAll --collectCoverage=false --testPathIgnorePatterns=integration --testPathIgnorePatterns=playwright", "test:withlog": "LOGLEVEL=trace npm run test", "test": "jest", "watch": "npx concurrently \"npm:*:watch\""