diff --git a/src/main/views/pluginsPopUps.test.js b/src/main/views/pluginsPopUps.test.js new file mode 100644 index 00000000..67472b58 --- /dev/null +++ b/src/main/views/pluginsPopUps.test.js @@ -0,0 +1,159 @@ +// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. + +import {shell} from 'electron'; + +import ServerViewState from 'app/serverViewState'; +import {parseURL} from 'common/utils/url'; +import ViewManager from 'main/views/viewManager'; + +import PluginsPopUpsManager from './pluginsPopUps'; + +jest.mock('electron', () => ({ + shell: { + openExternal: jest.fn(), + }, +})); + +jest.mock('main/views/viewManager', () => ({ + getView: jest.fn(), + getViewByWebContentsId: jest.fn(), + handleDeepLink: jest.fn(), +})); + +jest.mock('app/serverViewState', () => ({ + switchServer: jest.fn(), +})); + +jest.mock('main/windows/mainWindow', () => ({ + get: jest.fn(), + focus: jest.fn(), +})); + +const mockContextMenuReload = jest.fn(); +const mockContextMenuDispose = jest.fn(); +jest.mock('../contextMenu', () => { + return jest.fn().mockImplementation(() => { + return { + reload: mockContextMenuReload, + dispose: mockContextMenuDispose, + }; + }); +}); + +describe('PluginsPopUpsManager', () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + it('generateHandleCreateWindow', () => { + const handlers = {}; + const win = { + webContents: { + id: 45, + on: jest.fn((ev, handler) => { + handlers[ev] = handler; + }), + once: jest.fn((ev, handler) => { + handlers[ev] = handler; + }), + setWindowOpenHandler: jest.fn((handler) => { + handlers['window-open'] = handler; + }), + removeAllListeners: jest.fn(), + }, + once: jest.fn((ev, handler) => { + handlers[ev] = handler; + }), + }; + const details = { + url: 'about:blank', + }; + PluginsPopUpsManager.generateHandleCreateWindow(1)(win, details); + + expect(win.webContents.on).toHaveBeenNthCalledWith(1, 'will-redirect', handlers['will-redirect']); + expect(win.webContents.on).toHaveBeenNthCalledWith(2, 'will-navigate', handlers['will-navigate']); + expect(win.webContents.on).toHaveBeenNthCalledWith(3, 'did-start-navigation', handlers['did-start-navigation']); + expect(win.webContents.once).toHaveBeenCalledWith('render-process-gone', handlers['render-process-gone']); + expect(win.webContents.setWindowOpenHandler).toHaveBeenCalledWith(handlers['window-open']); + + expect(win.once).toHaveBeenCalledWith('closed', handlers.closed); + + expect(mockContextMenuReload).toHaveBeenCalledTimes(1); + + // Verify the popout has been added to the map + expect(PluginsPopUpsManager.popups).toHaveProperty('45', {parentId: 1, win}); + + // Verify redirects are disabled + const redirectEv = { + preventDefault: jest.fn(), + }; + handlers['will-redirect'](redirectEv); + expect(redirectEv.preventDefault).toHaveBeenCalled(); + + // Verify navigations are only allowed to the same url + const navigateEv = { + preventDefault: jest.fn(), + url: 'about:blank', + }; + handlers['will-navigate'](navigateEv); + expect(navigateEv.preventDefault).not.toHaveBeenCalled(); + navigateEv.url = 'http://localhost:8065'; + handlers['will-navigate'](navigateEv); + expect(navigateEv.preventDefault).toHaveBeenCalled(); + + navigateEv.preventDefault = jest.fn(); + navigateEv.url = 'about:blank'; + handlers['did-start-navigation'](navigateEv); + expect(navigateEv.preventDefault).not.toHaveBeenCalled(); + navigateEv.url = 'http://localhost:8065'; + handlers['did-start-navigation'](navigateEv); + expect(navigateEv.preventDefault).toHaveBeenCalled(); + + // Verify opening new windows is not allowed + expect(handlers['window-open']({url: ''})).toEqual({action: 'deny'}); + expect(shell.openExternal).not.toHaveBeenCalled(); + + // Verify internal link is routed in main view + ViewManager.getViewByWebContentsId.mockReturnValue({name: 'parent', webContentsId: 1, view: {server: {url: parseURL('http://localhost:8065'), id: 4545}}}); + expect(handlers['window-open']({url: 'http://localhost:8065/team/channel'})).toEqual({action: 'deny'}); + expect(shell.openExternal).not.toHaveBeenCalled(); + expect(ServerViewState.switchServer).toHaveBeenCalledWith(4545); + expect(ViewManager.handleDeepLink).toHaveBeenCalledWith(parseURL('http://localhost:8065/team/channel')); + + // Verify opening external links is allowed through browser + expect(handlers['window-open']({url: 'https://www.example.com'})).toEqual({action: 'deny'}); + expect(shell.openExternal).toHaveBeenCalledWith('https://www.example.com'); + + // Simulate render process gone + handlers['render-process-gone'](null, {reason: 'oom'}); + expect(win.webContents.removeAllListeners).toHaveBeenCalledTimes(1); + + // Throw case + win.webContents.removeAllListeners = jest.fn(() => { + throw new Error('failed'); + }); + handlers['render-process-gone'](null, {reason: 'clean-exit'}); + expect(win.webContents.removeAllListeners).toHaveBeenCalledTimes(1); + + // Close + handlers.closed(); + expect(mockContextMenuDispose).toHaveBeenCalledTimes(1); + + // Verify the popout reference has been deleted + expect(PluginsPopUpsManager.popups).toEqual({}); + }); + + it('handleNewWindow', () => { + // Anything but about:blank should not be allowed + expect(PluginsPopUpsManager.handleNewWindow(45, {url: ''})).toEqual({action: 'deny'}); + expect(PluginsPopUpsManager.handleNewWindow(45, {url: 'http://localhost:8065'})).toEqual({action: 'deny'}); + + // We should deny also if the parent view doesn't exist + expect(PluginsPopUpsManager.handleNewWindow(45, {url: 'about:blank'})).toEqual({action: 'deny'}); + + // Finally, we allow if URL is `about:blank` and a parent view exists + ViewManager.getViewByWebContentsId.mockReturnValue({name: 'parent', webContentsId: 1}); + expect(PluginsPopUpsManager.handleNewWindow(45, {url: 'about:blank'})).toEqual({action: 'allow'}); + }); +}); diff --git a/src/main/views/pluginsPopUps.ts b/src/main/views/pluginsPopUps.ts new file mode 100644 index 00000000..97ec1bbd --- /dev/null +++ b/src/main/views/pluginsPopUps.ts @@ -0,0 +1,137 @@ +// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. +import type { + BrowserWindow, + Event, + WebContentsWillNavigateEventParams, + WebContentsWillRedirectEventParams, + WebContentsDidStartNavigationEventParams, +} from 'electron'; +import {shell} from 'electron'; + +import ServerViewState from 'app/serverViewState'; +import {Logger} from 'common/log'; +import { + isTeamUrl, + parseURL, +} from 'common/utils/url'; +import ContextMenu from 'main/contextMenu'; +import ViewManager from 'main/views/viewManager'; +import {generateHandleConsoleMessage} from 'main/views/webContentEventsCommon'; +import MainWindow from 'main/windows/mainWindow'; + +const log = new Logger('PluginsPopUpsManager'); + +type PluginPopUp = { + parentId: number; + win: BrowserWindow; +} + +export class PluginsPopUpsManager { + popups: Record; + + constructor() { + this.popups = {}; + } + + generateHandleCreateWindow = (parentId: number) => (win: BrowserWindow, details: Electron.DidCreateWindowDetails) => { + const webContentsId = win.webContents.id; + + log.debug('created popup window', details.url, webContentsId); + this.popups[webContentsId] = { + parentId, + win, + }; + + // We take a conservative approach for the time being and disallow most events coming from popups: + // - Redirects + // - Navigation + // - Opening new windows + win.webContents.on('will-redirect', (ev: Event) => { + log.warn(`prevented popup window from redirecting to: ${ev.url}`); + ev.preventDefault(); + }); + win.webContents.on('will-navigate', (ev: Event) => { + if (ev.url === details.url) { + return; + } + + log.warn(`prevented popup window from navigating to: ${ev.url}`); + ev.preventDefault(); + }); + win.webContents.on('did-start-navigation', (ev: Event) => { + if (ev.url === details.url) { + return; + } + + log.warn(`prevented popup window from navigating to: ${ev.url}`); + ev.preventDefault(); + }); + win.webContents.setWindowOpenHandler(({url}): {action: 'deny'} => { + const parsedURL = parseURL(url); + if (!parsedURL) { + log.warn(`Ignoring non-url ${url}`); + return {action: 'deny'}; + } + + const serverView = ViewManager.getViewByWebContentsId(parentId)?.view; + + // We allow internal (i.e., same server) links to be routed as expected. + if (serverView && parsedURL && isTeamUrl(serverView.server.url, parsedURL, true)) { + ServerViewState.switchServer(serverView.server.id); + MainWindow.get()?.focus(); + ViewManager.handleDeepLink(parsedURL); + } else { + // We allow to open external links through browser. + shell.openExternal(url); + } + + log.warn(`prevented popup window from opening window to ${url}`); + + return {action: 'deny'}; + }); + + win.webContents.on('console-message', generateHandleConsoleMessage(log)); + + const contextMenu = new ContextMenu({}, win); + contextMenu.reload(); + + win.once('closed', () => { + log.debug('removing popup window', details.url, webContentsId); + Reflect.deleteProperty(this.popups, webContentsId); + contextMenu.dispose(); + }); + + win.webContents.once('render-process-gone', (_, details) => { + if (details.reason !== 'clean-exit') { + log.error('Renderer process for a webcontent is no longer available:', details.reason); + } + try { + win.webContents.removeAllListeners(); + } catch (e) { + log.error(`Error while trying to detach listeners, this might be ok if the process crashed: ${e}`); + } + }); + }; + + public handleNewWindow(parentId: number, details: Electron.HandlerDetails): {action: 'deny' | 'allow'} { + // Making extra explicit what we allow. This should already be enforced on + // the calling side. + if (details.url !== 'about:blank') { + log.warn(`prevented new window creation: ${details.url}`); + return {action: 'deny'}; + } + + // Make sure the parent view exists. + const parentView = ViewManager.getViewByWebContentsId(parentId); + if (!parentView) { + log.warn('handleNewWindow: parent view not found'); + return {action: 'deny'}; + } + + return {action: 'allow'}; + } +} + +const pluginsPopUpsManager = new PluginsPopUpsManager(); +export default pluginsPopUpsManager; diff --git a/src/main/views/webContentEvents.test.js b/src/main/views/webContentEvents.test.js index 7f4ae741..c64c7acb 100644 --- a/src/main/views/webContentEvents.test.js +++ b/src/main/views/webContentEvents.test.js @@ -9,7 +9,9 @@ import {getLevel} from 'common/log'; import ContextMenu from 'main/contextMenu'; import ViewManager from 'main/views/viewManager'; +import PluginsPopUpsManager from './pluginsPopUps'; import {WebContentsEventManager} from './webContentEvents'; +import {generateHandleConsoleMessage} from './webContentEventsCommon'; import allowProtocolDialog from '../allowProtocolDialog'; @@ -31,6 +33,11 @@ jest.mock('main/views/viewManager', () => ({ getViewByWebContentsId: jest.fn(), handleDeepLink: jest.fn(), })); + +jest.mock('main/views/pluginsPopUps', () => ({ + handleNewWindow: jest.fn(() => ({action: 'allow'})), +})); + jest.mock('../utils', () => ({ composeUserAgent: jest.fn(), })); @@ -179,6 +186,11 @@ describe('main/views/webContentsEvents', () => { expect(newWindow({url: 'devtools://aaaaaa.com'})).toStrictEqual({action: 'allow'}); }); + it('should defer about:blank to PluginsPopUpsManager', () => { + expect(newWindow({url: 'about:blank'})).toStrictEqual({action: 'allow'}); + expect(PluginsPopUpsManager.handleNewWindow).toHaveBeenCalledWith(1, {url: 'about:blank'}); + }); + it('should open invalid URIs in browser', () => { expect(newWindow({url: 'https://google.com/?^'})).toStrictEqual({action: 'deny'}); expect(shell.openExternal).toBeCalledWith('https://google.com/?^'); @@ -249,7 +261,7 @@ describe('main/views/webContentsEvents', () => { withPrefix: jest.fn().mockReturnThis(), }; webContentsEventManager.log = jest.fn().mockReturnValue(logObject); - const consoleMessage = webContentsEventManager.generateHandleConsoleMessage(); + const consoleMessage = generateHandleConsoleMessage(logObject); afterEach(() => { getLevel.mockReset(); diff --git a/src/main/views/webContentEvents.ts b/src/main/views/webContentEvents.ts index ab9830d1..8aa52b7f 100644 --- a/src/main/views/webContentEvents.ts +++ b/src/main/views/webContentEvents.ts @@ -1,13 +1,11 @@ // Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. // See LICENSE.txt for license information. -import path from 'path'; - import type {WebContents, Event} from 'electron'; import {BrowserWindow, shell} from 'electron'; import Config from 'common/config'; -import {Logger, getLevel} from 'common/log'; +import {Logger} from 'common/log'; import ServerManager from 'common/servers/serverManager'; import { isAdminUrl, @@ -28,10 +26,13 @@ import { } from 'common/utils/url'; import {flushCookiesStore} from 'main/app/utils'; import ContextMenu from 'main/contextMenu'; +import PluginsPopUpsManager from 'main/views/pluginsPopUps'; import ViewManager from 'main/views/viewManager'; import CallsWidgetWindow from 'main/windows/callsWidgetWindow'; import MainWindow from 'main/windows/mainWindow'; +import {generateHandleConsoleMessage} from './webContentEventsCommon'; + import {protocols} from '../../../electron-builder.json'; import allowProtocolDialog from '../allowProtocolDialog'; import {composeUserAgent} from '../utils'; @@ -40,13 +41,6 @@ type CustomLogin = { inProgress: boolean; } -enum ConsoleMessageLevel { - Verbose, - Info, - Warning, - Error -} - const log = new Logger('WebContentsEventManager'); const scheme = protocols && protocols[0] && protocols[0].schemes && protocols[0].schemes[0]; @@ -169,6 +163,11 @@ export class WebContentsEventManager { return {action: 'allow'}; } + // Allow plugins to open blank popup windows. + if (parsedURL.toString() === 'about:blank') { + return PluginsPopUpsManager.handleNewWindow(webContentsId, details); + } + // Check for custom protocol if (parsedURL.protocol !== 'http:' && parsedURL.protocol !== 'https:' && parsedURL.protocol !== `${scheme}:`) { allowProtocolDialog.handleDialogEvent(parsedURL.protocol, details.url); @@ -296,27 +295,6 @@ export class WebContentsEventManager { }; }; - private generateHandleConsoleMessage = (webContentsId: number) => (_: Event, level: number, message: string, line: number, sourceId: string) => { - const wcLog = this.log(webContentsId).withPrefix('renderer'); - let logFn = wcLog.debug; - switch (level) { - case ConsoleMessageLevel.Error: - logFn = wcLog.error; - break; - case ConsoleMessageLevel.Warning: - logFn = wcLog.warn; - break; - } - - // Only include line entries if we're debugging - const entries = [message]; - if (['debug', 'silly'].includes(getLevel())) { - entries.push(`(${path.basename(sourceId)}:${line})`); - } - - logFn(...entries); - }; - removeWebContentsListeners = (id: number) => { if (this.listeners[id]) { this.listeners[id](); @@ -352,7 +330,11 @@ export class WebContentsEventManager { const newWindow = this.generateNewWindowListener(contents.id, spellcheck); contents.setWindowOpenHandler(newWindow); - const consoleMessage = this.generateHandleConsoleMessage(contents.id); + // Defer handling of new popup windows to PluginsPopUpsManager. These still need to be + // previously allowed from generateNewWindowListener through PluginsPopUpsManager.handleNewWindow. + contents.on('did-create-window', PluginsPopUpsManager.generateHandleCreateWindow(contents.id)); + + const consoleMessage = generateHandleConsoleMessage(this.log(contents.id)); contents.on('console-message', consoleMessage); addListeners?.(contents); diff --git a/src/main/views/webContentEventsCommon.ts b/src/main/views/webContentEventsCommon.ts new file mode 100644 index 00000000..13f2e404 --- /dev/null +++ b/src/main/views/webContentEventsCommon.ts @@ -0,0 +1,36 @@ +// Copyright (c) 2016-present Mattermost, Inc. All Rights Reserved. +// See LICENSE.txt for license information. +import path from 'path'; + +import type {Event} from 'electron'; + +import type {Logger} from 'common/log'; +import {getLevel} from 'common/log'; + +enum ConsoleMessageLevel { + Verbose, + Info, + Warning, + Error +} + +export const generateHandleConsoleMessage = (log: Logger) => (_: Event, level: number, message: string, line: number, sourceId: string) => { + const wcLog = log.withPrefix('renderer'); + let logFn = wcLog.debug; + switch (level) { + case ConsoleMessageLevel.Error: + logFn = wcLog.error; + break; + case ConsoleMessageLevel.Warning: + logFn = wcLog.warn; + break; + } + + // Only include line entries if we're debugging + const entries = [message]; + if (['debug', 'silly'].includes(getLevel())) { + entries.push(`(${path.basename(sourceId)}:${line})`); + } + + logFn(...entries); +};