Merge pull request #15204 from brave/fix/15134

Block privileged URL loads in context menu only

app/browser/menu.js

@@ -70,7 +70,7 @@ const createFileSubmenu = () => {
               appActions.createTabRequested({
                 url: fileUrl(path),
                 windowId: focusedWindow.id
-              }, false, false, false, true)
+              })
             })
           }
         })

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

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 {

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

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

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

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'
   },
 
   /**

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