diff --git a/app/browser/menu.js b/app/browser/menu.js index 062cf817e70..f436d83461b 100644 --- a/app/browser/menu.js +++ b/app/browser/menu.js @@ -70,7 +70,7 @@ const createFileSubmenu = () => { appActions.createTabRequested({ url: fileUrl(path), windowId: focusedWindow.id - }, false, false, false, true) + }) }) } }) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index c8478dda04b..06653904cb5 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -37,7 +37,6 @@ const {getFlashResourceId} = require('../../../js/flash') const {l10nErrorText} = require('../../common/lib/httpUtil') const flash = require('../../../js/flash') const {isSourceAboutUrl, isTargetAboutUrl, isNavigatableAboutPage} = require('../../../js/lib/appUrlUtil') -const {isFileScheme, openableByContextMenu} = require('../../../js/lib/urlutil') const {shouldDebugTabEvents} = require('../../cmdLine') const getWebRTCPolicy = (state, tabId) => { @@ -250,11 +249,6 @@ const tabsReducer = (state, action, immutableAction) => { windows.focus(windowId) } const url = action.getIn(['createProperties', 'url']) - if (url && (!openableByContextMenu(url) || - (isFileScheme(url) && !action.get('allowFile')))) { - // Don't allow 'open in new tab' to open file:// URLs for security - action = action.setIn(['createProperties', 'url'], 'about:blank') - } setImmediate(() => { if (action.get('activateIfOpen') || ((isSourceAboutUrl(url) || isTargetAboutUrl(url)) && isNavigatableAboutPage(url))) { diff --git a/app/browser/reducers/windowsReducer.js b/app/browser/reducers/windowsReducer.js index c68fb4cd53d..73abc4e31eb 100644 --- a/app/browser/reducers/windowsReducer.js +++ b/app/browser/reducers/windowsReducer.js @@ -22,7 +22,6 @@ const {makeImmutable, isImmutable} = require('../../common/state/immutableUtil') const electron = require('electron') const BrowserWindow = electron.BrowserWindow const firstDefinedValue = require('../../../js/lib/functional').firstDefinedValue -const {isFileScheme, openableByContextMenu} = require('../../../js/lib/urlutil') const settings = require('../../../js/constants/settings') const getSetting = require('../../../js/settings').getSetting @@ -267,11 +266,6 @@ const handleCreateWindowAction = (state, action = Immutable.Map()) => { if (Array.isArray(frameOpts)) { frames = frameOpts } else { - // Don't allow 'open in new window' to open a file:// URL for - // security reasons - if (isFileScheme(frameOpts.location) || !openableByContextMenu(frameOpts.location)) { - frameOpts.location = 'about:blank' - } frames = [ frameOpts ] } } else { diff --git a/app/renderer/reducers/contextMenuReducer.js b/app/renderer/reducers/contextMenuReducer.js index 029523a8280..f229a04e420 100644 --- a/app/renderer/reducers/contextMenuReducer.js +++ b/app/renderer/reducers/contextMenuReducer.js @@ -43,6 +43,8 @@ const {makeImmutable, isMap} = require('../../common/state/immutableUtil') const {getCurrentWindow, getCurrentWindowId} = require('../../renderer/currentWindow') const {getSetting} = require('../../../js/settings') +const sanitizeUrl = urlUtil.sanitizeForContextMenu + const validateAction = function (action) { action = makeImmutable(action) assert.ok(isMap(action), 'action must be an Immutable.Map') @@ -117,7 +119,7 @@ const openInNewTabMenuItem = (url, partitionNumber, openerTabId) => { click: () => { for (let i = 0; i < url.length; ++i) { appActions.createTabRequested({ - url: url[i], + url: sanitizeUrl(url[i]), partitionNumber: partitionNumber[i], openerTabId, active @@ -130,7 +132,7 @@ const openInNewTabMenuItem = (url, partitionNumber, openerTabId) => { label: locale.translation('openInNewTab'), click: () => { appActions.createTabRequested({ - url, + url: sanitizeUrl(url), partitionNumber, openerTabId, active @@ -148,7 +150,7 @@ const openInNewPrivateTabMenuItem = (url, openerTabId, isTor) => { click: () => { for (let i = 0; i < url.length; ++i) { appActions.createTabRequested({ - url: url[i], + url: sanitizeUrl(url[i]), isPrivate: true, isTor, openerTabId, @@ -162,7 +164,7 @@ const openInNewPrivateTabMenuItem = (url, openerTabId, isTor) => { label: locale.translation(isTor ? 'openInNewTorTab' : 'openInNewPrivateTab'), click: () => { appActions.createTabRequested({ - url, + url: sanitizeUrl(url), isPrivate: true, isTor, openerTabId, @@ -181,7 +183,7 @@ const openInNewSessionTabMenuItem = (url, openerTabId) => { click: () => { for (let i = 0; i < url.length; ++i) { appActions.createTabRequested({ - url: url[i], + url: sanitizeUrl(url[i]), isPartitioned: true, openerTabId, active @@ -194,7 +196,7 @@ const openInNewSessionTabMenuItem = (url, openerTabId) => { label: locale.translation('openInNewSessionTab'), click: () => { appActions.createTabRequested({ - url, + url: sanitizeUrl(url), isPartitioned: true, openerTabId, active @@ -208,7 +210,10 @@ const openInNewWindowMenuItem = (location, partitionNumber) => { return { label: locale.translation('openInNewWindow'), click: () => { - appActions.newWindow({ location, partitionNumber }) + appActions.newWindow({ + location: sanitizeUrl(location), + partitionNumber + }) } } } @@ -557,7 +562,7 @@ const onLongBackHistory = (state, action) => { click: function (e) { if (eventUtil.isForSecondaryAction(e)) { appActions.createTabRequested({ - url, + url: sanitizeUrl(url), partitionNumber: action.get('partitionNumber'), active: !!e.shiftKey }) @@ -610,7 +615,7 @@ const onLongForwardHistory = (state, action) => { click: function (e) { if (eventUtil.isForSecondaryAction(e)) { appActions.createTabRequested({ - url, + url: sanitizeUrl(url), partitionNumber: action.get('partitionNumber'), active: !!e.shiftKey }) diff --git a/js/actions/appActions.js b/js/actions/appActions.js index aec7c43cccc..61a0231ccfa 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -269,15 +269,13 @@ const appActions = { * switch to it instead of creating a new one * @param {Boolean} isRestore when true, won't try to activate the new tab, even if the user preference indicates to * @param {Boolean} focusWindow - * @param {Boolean} allowFile - When true, allows file:// URLs to be opened */ - createTabRequested: function (createProperties, activateIfOpen = false, isRestore = false, focusWindow = false, allowFile = false) { + createTabRequested: function (createProperties, activateIfOpen = false, isRestore = false, focusWindow = false) { dispatch({ actionType: appConstants.APP_CREATE_TAB_REQUESTED, createProperties, activateIfOpen, isRestore, - allowFile, focusWindow }) }, diff --git a/js/contextMenus.js b/js/contextMenus.js index b4a0f1c7cac..d487b31ee67 100644 --- a/js/contextMenus.js +++ b/js/contextMenus.js @@ -43,6 +43,8 @@ const ledgerUtil = require('../app/common/lib/ledgerUtil') const isDarwin = platformUtil.isDarwin() const isLinux = platformUtil.isLinux() +const sanitizeUrl = urlUtil.sanitizeForContextMenu + /** * Gets the correct search URL for the current frame. * @param {Immutable.Map} activeFrame - currently active frame @@ -764,7 +766,7 @@ const openInNewTabMenuItem = (url, partitionNumber, openerTabId) => { click: () => { for (let i = 0; i < url.length; ++i) { appActions.createTabRequested({ - url: url[i], + url: sanitizeUrl(url[i]), partitionNumber: partitionNumber[i], openerTabId, active @@ -777,7 +779,7 @@ const openInNewTabMenuItem = (url, partitionNumber, openerTabId) => { label: locale.translation('openInNewTab'), click: () => { appActions.createTabRequested({ - url, + url: sanitizeUrl(url), partitionNumber, openerTabId, active @@ -804,7 +806,7 @@ const openInNewPrivateTabMenuItem = (url, openerTabId, isTor) => { click: () => { for (let i = 0; i < url.length; ++i) { appActions.createTabRequested({ - url: url[i], + url: sanitizeUrl(url[i]), isPrivate: true, isTor, openerTabId, @@ -818,7 +820,7 @@ const openInNewPrivateTabMenuItem = (url, openerTabId, isTor) => { label: locale.translation(isTor ? 'openInNewTorTab' : 'openInNewPrivateTab'), click: () => { appActions.createTabRequested({ - url, + url: sanitizeUrl(url), isPrivate: true, isTor, openerTabId, @@ -833,7 +835,12 @@ const openInNewWindowMenuItem = (location, isPrivate, partitionNumber, isTor) => return { label: locale.translation('openInNewWindow'), click: () => { - appActions.newWindow({ location, isPrivate, isTor, partitionNumber }) + appActions.newWindow({ + location: sanitizeUrl(location), + isPrivate, + isTor, + partitionNumber + }) } } } @@ -846,7 +853,7 @@ const openInNewSessionTabMenuItem = (url, openerTabId) => { click: (item) => { for (let i = 0; i < url.length; ++i) { appActions.createTabRequested({ - url: url[i], + url: sanitizeUrl(url[i]), isPartitioned: true, openerTabId, active @@ -859,7 +866,7 @@ const openInNewSessionTabMenuItem = (url, openerTabId) => { label: locale.translation('openInNewSessionTab'), click: (item) => { appActions.createTabRequested({ - url, + url: sanitizeUrl(url), isPartitioned: true, openerTabId, active @@ -912,7 +919,7 @@ const searchSelectionMenuItem = (location) => { const isPrivate = frame.get('isPrivate') const isTor = frameStateUtil.isTor(frame) appActions.createTabRequested({ - url: searchUrl, + url: sanitizeUrl(searchUrl), isPrivate, isTor, partitionNumber: frame.get('partitionNumber'), @@ -999,7 +1006,7 @@ function mainTemplateInit (nodeProps, frame, tab) { click: (item) => { if (nodeProps.srcURL) { appActions.createTabRequested({ - url: nodeProps.srcURL, + url: sanitizeUrl(nodeProps.srcURL), openerTabId: frame.get('tabId'), isPrivate, isTor, @@ -1027,7 +1034,7 @@ function mainTemplateInit (nodeProps, frame, tab) { label: locale.translation('searchImage'), click: () => { appActions.createTabRequested({ - url: searchUrl.replace('?q', 'byimage?image_url'), + url: sanitizeUrl(searchUrl.replace('?q', 'byimage?image_url')), isPrivate, isTor, partitionNumber: frame.get('partitionNumber') diff --git a/js/lib/urlutil.js b/js/lib/urlutil.js index 9a097246f96..b4eaf328c91 100644 --- a/js/lib/urlutil.js +++ b/js/lib/urlutil.js @@ -424,17 +424,20 @@ const UrlUtil = { /** * Checks if URL is safe to open via 'open in new tab/window' context menu + * If so, returns the URL. If not, returns about:blank. * @param {string} url - URL to check - * @return {boolean} + * @return {string} */ - openableByContextMenu: function (url) { + sanitizeForContextMenu: function (url) { if (!url) { - return true + return url } const protocol = urlParse(url).protocol - // file: is untrusted but handled in a separate check - return ['http:', 'https:', 'ws:', 'wss:', 'magnet:', 'file:', 'data:', - 'blob:', 'about:', 'chrome-extension:', 'view-source:'].includes(protocol) + if (['http:', 'https:', 'ws:', 'wss:', 'magnet:', 'data:', 'blob:', + 'about:', 'chrome-extension:', 'view-source:'].includes(protocol)) { + return url + } + return 'about:blank' }, /** diff --git a/test/unit/lib/urlutilTestComponents.js b/test/unit/lib/urlutilTestComponents.js index 3c7adcfbb1d..c764ccc8a64 100644 --- a/test/unit/lib/urlutilTestComponents.js +++ b/test/unit/lib/urlutilTestComponents.js @@ -1,6 +1,8 @@ // lazy load requires for dual use in and outside muon const urlUtil = () => require('../../../js/lib/urlutil') +const blank = 'about:blank' + module.exports = { 'getScheme': { 'null for empty': (test) => { @@ -328,30 +330,36 @@ module.exports = { } }, - 'openableByContextMenu': { - 'returns true when input:': { - 'is an absolute file path with scheme': (test) => { - test.equal(urlUtil().openableByContextMenu('file:///file/path/to/file'), true) - }, + 'sanitizeForContextMenu': { + 'returns original URL when input:': { 'is http': (test) => { - test.equal(urlUtil().openableByContextMenu('http://example.com'), true) + const url = 'http://example.com' + test.equal(urlUtil().sanitizeForContextMenu(url), url) }, 'is https': (test) => { - test.equal(urlUtil().openableByContextMenu('HTtpS://brave.com/?abc=1#test'), true) + const url = 'HTtpS://brave.com/?abc=1#test' + test.equal(urlUtil().sanitizeForContextMenu(url), url) }, 'is about:blank': (test) => { - test.equal(urlUtil().openableByContextMenu('about:blank'), true) + const url = 'about:blank' + test.equal(urlUtil().sanitizeForContextMenu(url), url) }, 'is empty': (test) => { - test.equal(urlUtil().openableByContextMenu(), true) + test.equal(urlUtil().sanitizeForContextMenu(), undefined) } }, - 'returns false when input:': { + 'returns blank when input:': { + 'is an absolute file path with scheme': (test) => { + test.equal(urlUtil().sanitizeForContextMenu('file:///file/path/to/file'), blank) + }, + 'is file without scheme': (test) => { + test.equal(urlUtil().sanitizeForContextMenu('/file/path/to/file'), blank) + }, 'is chrome:': (test) => { - test.equal(urlUtil().openableByContextMenu('chrome://brave/etc/passwd'), false) + test.equal(urlUtil().sanitizeForContextMenu('chrome://brave/etc/passwd'), blank) }, 'is ssh:': (test) => { - test.equal(urlUtil().openableByContextMenu('ssh://test@127.0.0.1'), false) + test.equal(urlUtil().sanitizeForContextMenu('ssh://test@127.0.0.1'), blank) } } },