Skip to content
This repository was archived by the owner on Dec 11, 2019. It is now read-only.

second fix for URL open in new tab #15047

Merged
merged 1 commit into from
Aug 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions app/browser/reducers/tabsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ 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} = require('../../../js/lib/urlutil')
const {isFileScheme, openableByContextMenu} = require('../../../js/lib/urlutil')
const {shouldDebugTabEvents} = require('../../cmdLine')

const getWebRTCPolicy = (state, tabId) => {
Expand Down Expand Up @@ -250,7 +250,8 @@ const tabsReducer = (state, action, immutableAction) => {
windows.focus(windowId)
}
const url = action.getIn(['createProperties', 'url'])
if (isFileScheme(url) && !action.get('allowFile')) {
if (!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')
}
Expand Down
4 changes: 2 additions & 2 deletions app/browser/reducers/windowsReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ const {makeImmutable, isImmutable} = require('../../common/state/immutableUtil')
const electron = require('electron')
const BrowserWindow = electron.BrowserWindow
const firstDefinedValue = require('../../../js/lib/functional').firstDefinedValue
const {isFileScheme} = require('../../../js/lib/urlutil')
const {isFileScheme, openableByContextMenu} = require('../../../js/lib/urlutil')
const settings = require('../../../js/constants/settings')
const getSetting = require('../../../js/settings').getSetting

Expand Down Expand Up @@ -269,7 +269,7 @@ const handleCreateWindowAction = (state, action = Immutable.Map()) => {
} else {
// Don't allow 'open in new window' to open a file:// URL for
// security reasons
if (isFileScheme(frameOpts.location)) {
if (isFileScheme(frameOpts.location) || !openableByContextMenu(frameOpts.location)) {
frameOpts.location = 'about:blank'
}
frames = [ frameOpts ]
Expand Down
15 changes: 15 additions & 0 deletions js/lib/urlutil.js
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,21 @@ const UrlUtil = {
return urlParse(url).protocol === 'file:'
},

/**
* Checks if URL is safe to open via 'open in new tab/window' context menu
* @param {string} url - URL to check
* @return {boolean}
*/
openableByContextMenu: function (url) {
if (!url) {
return false
}
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:'].includes(protocol)
},

/**
* Gets the origin of a given URL
* @param {string} url The URL to get the origin from
Expand Down
28 changes: 28 additions & 0 deletions test/unit/lib/urlutilTestComponents.js
Original file line number Diff line number Diff line change
Expand Up @@ -346,6 +346,34 @@ 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)
},
'is http': (test) => {
test.equal(urlUtil().openableByContextMenu('http://example.com'), true)
},
'is https': (test) => {
test.equal(urlUtil().openableByContextMenu('HTtpS://brave.com/?abc=1#test'), true)
},
'is about:blank': (test) => {
test.equal(urlUtil().openableByContextMenu('about:blank'), true)
}
},
'returns false when input:': {
'is chrome:': (test) => {
test.equal(urlUtil().openableByContextMenu('chrome://brave/etc/passwd'), false)
},
'is ssh:': (test) => {
test.equal(urlUtil().openableByContextMenu('ssh://[email protected]'), false)
},
'is null': (test) => {
test.equal(urlUtil().openableByContextMenu(null), false)
}
}
},

'getDisplayHost': {
'url is http': (test) => {
const result = urlUtil().getDisplayHost('http://brave.com')
Expand Down