Add validation to opening URLs in external desktop handler (fix #1459)

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({

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);

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();
+  });
+});

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&regexp=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<void> {
-  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);
 }
 

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);
   });

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<void> {
-  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;

app/src/helpers/windowHelpers.ts

@@ -23,12 +23,14 @@ export function adjustWindowZoom(adjustment: number): void {
   });
 }
 
-export function blockExternalURL(url: string): Promise<MessageBoxReturnValue> {
+export function showNavigationBlockedMessage(
+  message: string,
+): Promise<MessageBoxReturnValue> {
   return new Promise((resolve, reject) => {
     withFocusedWindow((focusedWindow) => {
       dialog
         .showMessageBox(focusedWindow, {
-          message: `Cannot navigate to external URL: ${url}`,
+          message,
           type: 'error',
           title: 'Navigation blocked',
         })

app/src/main.ts

@@ -438,14 +438,14 @@ app.on(
   },
 );
 
-app.on('browser-window-blur', (event: Event, window: BrowserWindow) => {
+app.on('browser-window-blur', () => {
-  log.debug('app.browser-window-blur', { event, window });
+  log.debug('app.browser-window-blur');
 });
 
-app.on('browser-window-created', (event: Event, window: BrowserWindow) => {
+app.on('browser-window-created', () => {
-  log.debug('app.browser-window-created', { event, window });
+  log.debug('app.browser-window-created');
 });
 
-app.on('browser-window-focus', (event: Event, window: BrowserWindow) => {
+app.on('browser-window-focus', () => {
-  log.debug('app.browser-window-focus', { event, window });
+  log.debug('app.browser-window-focus');
 });

app/src/mocks/electron.ts

@@ -144,10 +144,17 @@ class MockWebRequest {
 
 class InternalEmitter extends EventEmitter {}
 
+const mockShell = {
+  openExternal(url: string, options?: unknown): Promise<void> {
+    return new Promise((resolve) => resolve());
+  },
+};
+
 export {
   MockDialog as dialog,
   MockBrowserWindow as BrowserWindow,
   MockSession as Session,
   MockWebContents as WebContents,
   MockWebRequest as WebRequest,
+  mockShell as shell,
 };

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\""