[MM-60226] Allow plugins to open blank popup windows (#3136)

* Allow plugins to open blank popup windows

* Simplify with arrow function method

* Handle console messages and render process exit

* Allow opening external links in browser

* Allow routing internal links

* Fix test failure

* Remove allow for potential returns

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

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<number, PluginPopUp>;
+
+    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<WebContentsWillRedirectEventParams>) => {
+            log.warn(`prevented popup window from redirecting to: ${ev.url}`);
+            ev.preventDefault();
+        });
+        win.webContents.on('will-navigate', (ev: Event<WebContentsWillNavigateEventParams>) => {
+            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<WebContentsDidStartNavigationEventParams>) => {
+            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;

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

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

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