From cd968b729717310daa6bee8ffd1c396cebc8e2d9 Mon Sep 17 00:00:00 2001 From: bridiver Date: Mon, 11 Sep 2017 10:12:56 -0700 Subject: [PATCH 1/4] handle an array of permission types fix #10864 requires muon 4.3.19+ --- app/filtering.js | 142 +++++++++--------- .../components/main/notificationItem.js | 9 +- 2 files changed, 79 insertions(+), 72 deletions(-) diff --git a/app/filtering.js b/app/filtering.js index bfb23b5734b..9df76e1825c 100644 --- a/app/filtering.js +++ b/app/filtering.js @@ -389,7 +389,7 @@ function registerPermissionHandler (session, partition) { const isPrivate = module.exports.isPrivate(partition) // Keep track of per-site permissions granted for this session. let permissions = null - session.setPermissionRequestHandler((origin, mainFrameUrl, permission, cb) => { + session.setPermissionRequestHandler((origin, mainFrameUrl, permissionTypes, cb) => { if (!permissions) { permissions = { media: { @@ -419,34 +419,7 @@ function registerPermissionHandler (session, partition) { } } - if (!permissions[permission]) { - console.warn('WARNING: got unregistered permission request', permission) - cb(false) - return - } - - // The Torrent Viewer extension is always allowed to show fullscreen media - if (permission === 'fullscreen' && - origin.startsWith('chrome-extension://' + config.torrentExtensionId)) { - cb(true) - return - } - - // Always allow fullscreen if setting is ON - const alwaysAllowFullscreen = module.exports.alwaysAllowFullscreen() === fullscreenOption.ALWAYS_ALLOW - if (permission === 'fullscreen' && alwaysAllowFullscreen) { - cb(true) - return - } - - // The Brave extension and PDFJS are always allowed to open files in an external app - if (permission === 'openExternal' && ( - origin.startsWith('chrome-extension://' + config.PDFJSExtensionId) || - origin.startsWith('chrome-extension://' + config.braveExtensionId))) { - cb(true) - return - } - + // TODO(bridiver) - the permission handling should be converted to an action because we should never call `appStore.getState()` // Check whether there is a persistent site setting for this host const appState = appStore.getState() let settings @@ -474,50 +447,81 @@ function registerPermissionHandler (session, partition) { tempSettings = siteSettings.getSiteSettingsForURL(appState.get('temporarySiteSettings'), origin) } - const permissionName = permission + 'Permission' - let isAllowed - if (settings) { - isAllowed = settings.get(permissionName) - } - // Private tabs inherit settings from normal tabs, but not vice versa. - if (isPrivate && tempSettings) { - isAllowed = tempSettings.get(permissionName) - } - if (typeof isAllowed === 'boolean') { - cb(isAllowed) - return - } + let response = [] + for (let i = 0; i < permissionTypes.length; i++) { + const permission = permissionTypes[i] + if (!permissions[permission]) { + console.warn('WARNING: got unregistered permission request', permission) + response.push(false) + } - // Display 'Brave Browser' if the origin is null; ex: when a mailto: link - // is opened in a new tab via right-click - const message = locale.translation('permissionMessage').replace(/{{\s*host\s*}}/, origin || 'Brave Browser').replace(/{{\s*permission\s*}}/, permissions[permission].action) + // The Torrent Viewer extension is always allowed to show fullscreen media + if (permission === 'fullscreen' && + origin.startsWith('chrome-extension://' + config.torrentExtensionId)) { + response.push(true) + } - // If this is a duplicate, clear the previous callback and use the new one - if (permissionCallbacks[message]) { - permissionCallbacks[message](0, false) - } + // Always allow fullscreen if setting is ON + const alwaysAllowFullscreen = module.exports.alwaysAllowFullscreen() === fullscreenOption.ALWAYS_ALLOW + if (permission === 'fullscreen' && alwaysAllowFullscreen) { + response.push(true) + } - appActions.showNotification({ - buttons: [ - {text: locale.translation('deny')}, - {text: locale.translation('allow')} - ], - frameOrigin: getOrigin(mainFrameUrl), - options: { - persist: !!origin - }, - message - }) + // The Brave extension and PDFJS are always allowed to open files in an external app + if (permission === 'openExternal' && ( + origin.startsWith('chrome-extension://' + config.PDFJSExtensionId) || + origin.startsWith('chrome-extension://' + config.braveExtensionId))) { + response.push(true) + } + + const permissionName = permission + 'Permission' + let isAllowed + if (settings) { + isAllowed = settings.get(permissionName) + } + // Private tabs inherit settings from normal tabs, but not vice versa. + if (isPrivate && tempSettings) { + isAllowed = tempSettings.get(permissionName) + } + if (typeof isAllowed === 'boolean') { + response.push(isAllowed) + } + + // Display 'Brave Browser' if the origin is null; ex: when a mailto: link + // is opened in a new tab via right-click + const message = locale.translation('permissionMessage').replace(/{{\s*host\s*}}/, origin || 'Brave Browser').replace(/{{\s*permission\s*}}/, permissions[permission].action) + + // If this is a duplicate, clear the previous callback and use the new one + if (permissionCallbacks[message]) { + permissionCallbacks[message](0, false, i) + } - permissionCallbacks[message] = (buttonIndex, persist) => { - permissionCallbacks[message] = null - // hide the notification if this was triggered automatically - appActions.hideNotification(message) - const result = !!(buttonIndex) - cb(result) - if (persist) { - // remember site setting for this host - appActions.changeSiteSetting(origin, permission + 'Permission', result, isPrivate) + appActions.showNotification({ + buttons: [ + {text: locale.translation('deny')}, + {text: locale.translation('allow')} + ], + frameOrigin: getOrigin(mainFrameUrl), + options: { + persist: !!origin, + index: i + }, + message + }) + + permissionCallbacks[message] = (buttonIndex, persist, index) => { + // hide the notification if this was triggered automatically + appActions.hideNotification(message) + const result = !!(buttonIndex) + response[index] = result + if (persist) { + // remember site setting for this host + appActions.changeSiteSetting(origin, permission + 'Permission', result, isPrivate) + } + if (response.length === permissionTypes.length) { + permissionCallbacks[message] = null + cb(response) + } } } }) diff --git a/app/renderer/components/main/notificationItem.js b/app/renderer/components/main/notificationItem.js index 3ba54190f8b..847f948cbe9 100644 --- a/app/renderer/components/main/notificationItem.js +++ b/app/renderer/components/main/notificationItem.js @@ -32,19 +32,21 @@ class NotificationItem extends React.Component { clickHandler (buttonIndex) { if (this.props.nonce) { - ipc.emit( + ipc.send( messages.NOTIFICATION_RESPONSE + this.props.nonce, {}, this.props.message, buttonIndex, - this.checkbox ? this.checkbox.checked : false + this.checkbox ? this.checkbox.checked : false, + this.props.index ) } else { ipc.send( messages.NOTIFICATION_RESPONSE, this.props.message, buttonIndex, - this.checkbox ? this.checkbox.checked : false + this.checkbox ? this.checkbox.checked : false, + this.props.index ) } } @@ -74,6 +76,7 @@ class NotificationItem extends React.Component { props.advancedLink = notification.getIn(['options', 'advancedLink']) props.persist = notification.getIn(['options', 'persist']) props.nonce = notification.getIn(['options', 'nonce']) + props.index = notification.getIn(['options', 'index']) return props } From cb4c6d3e047b4897bb12908d6ec5cb35be216553 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Wed, 13 Sep 2017 09:39:04 -0400 Subject: [PATCH 2/4] Don't push more than possible for permissionTypes This addresses review feedback for #10864 --- app/filtering.js | 46 ++++++++++++++++++++-------------------------- 1 file changed, 20 insertions(+), 26 deletions(-) diff --git a/app/filtering.js b/app/filtering.js index 9df76e1825c..866d8fabaa5 100644 --- a/app/filtering.js +++ b/app/filtering.js @@ -450,41 +450,35 @@ function registerPermissionHandler (session, partition) { let response = [] for (let i = 0; i < permissionTypes.length; i++) { const permission = permissionTypes[i] + const alwaysAllowFullscreen = module.exports.alwaysAllowFullscreen() === fullscreenOption.ALWAYS_ALLOW if (!permissions[permission]) { console.warn('WARNING: got unregistered permission request', permission) response.push(false) - } - - // The Torrent Viewer extension is always allowed to show fullscreen media - if (permission === 'fullscreen' && + } else if (permission === 'fullscreen' && + // The Torrent Viewer extension is always allowed to show fullscreen media origin.startsWith('chrome-extension://' + config.torrentExtensionId)) { response.push(true) - } - - // Always allow fullscreen if setting is ON - const alwaysAllowFullscreen = module.exports.alwaysAllowFullscreen() === fullscreenOption.ALWAYS_ALLOW - if (permission === 'fullscreen' && alwaysAllowFullscreen) { + } else if (permission === 'fullscreen' && alwaysAllowFullscreen) { + // Always allow fullscreen if setting is ON response.push(true) - } - - // The Brave extension and PDFJS are always allowed to open files in an external app - if (permission === 'openExternal' && ( + } else if (permission === 'openExternal' && ( + // The Brave extension and PDFJS are always allowed to open files in an external app origin.startsWith('chrome-extension://' + config.PDFJSExtensionId) || origin.startsWith('chrome-extension://' + config.braveExtensionId))) { response.push(true) - } - - const permissionName = permission + 'Permission' - let isAllowed - if (settings) { - isAllowed = settings.get(permissionName) - } - // Private tabs inherit settings from normal tabs, but not vice versa. - if (isPrivate && tempSettings) { - isAllowed = tempSettings.get(permissionName) - } - if (typeof isAllowed === 'boolean') { - response.push(isAllowed) + } else { + const permissionName = permission + 'Permission' + let isAllowed + if (settings) { + isAllowed = settings.get(permissionName) + } + // Private tabs inherit settings from normal tabs, but not vice versa. + if (isPrivate && tempSettings) { + isAllowed = tempSettings.get(permissionName) + } + if (typeof isAllowed === 'boolean') { + response.push(isAllowed) + } } // Display 'Brave Browser' if the origin is null; ex: when a mailto: link From 95d4b7d77016b76adadd064695f4e1ae096d135d Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Wed, 13 Sep 2017 11:23:09 -0400 Subject: [PATCH 3/4] Set correct permission notification index Auditors: @bridiver --- app/filtering.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/filtering.js b/app/filtering.js index 866d8fabaa5..83608e677f7 100644 --- a/app/filtering.js +++ b/app/filtering.js @@ -487,7 +487,7 @@ function registerPermissionHandler (session, partition) { // If this is a duplicate, clear the previous callback and use the new one if (permissionCallbacks[message]) { - permissionCallbacks[message](0, false, i) + permissionCallbacks[message](0, false) } appActions.showNotification({ @@ -503,7 +503,11 @@ function registerPermissionHandler (session, partition) { message }) - permissionCallbacks[message] = (buttonIndex, persist, index) => { + // Use a closure here for the index instead of passing an index to the + // function because ipcMain.on(messages.NOTIFICATION_RESPONSE above + // calls into the callback without knowing an index. + const index = i + permissionCallbacks[message] = (buttonIndex, persist) => { // hide the notification if this was triggered automatically appActions.hideNotification(message) const result = !!(buttonIndex) From 71ed6819a8893b1658af4f25da165b8154bdbe01 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Thu, 14 Sep 2017 16:30:00 -0400 Subject: [PATCH 4/4] Fix persistent permissions Already covered by this test: ``` 1) notificationBar permissions can accept permission request persistently: Promise was rejected with the following reason: timeout ``` Fix #10957 --- app/filtering.js | 65 +++++++++++++++++++++++++++--------------------- 1 file changed, 36 insertions(+), 29 deletions(-) diff --git a/app/filtering.js b/app/filtering.js index 83608e677f7..76220afbacf 100644 --- a/app/filtering.js +++ b/app/filtering.js @@ -449,6 +449,7 @@ function registerPermissionHandler (session, partition) { let response = [] for (let i = 0; i < permissionTypes.length; i++) { + const responseSizeThisIteration = response.length const permission = permissionTypes[i] const alwaysAllowFullscreen = module.exports.alwaysAllowFullscreen() === fullscreenOption.ALWAYS_ALLOW if (!permissions[permission]) { @@ -490,38 +491,44 @@ function registerPermissionHandler (session, partition) { permissionCallbacks[message](0, false) } - appActions.showNotification({ - buttons: [ - {text: locale.translation('deny')}, - {text: locale.translation('allow')} - ], - frameOrigin: getOrigin(mainFrameUrl), - options: { - persist: !!origin, - index: i - }, - message - }) - - // Use a closure here for the index instead of passing an index to the - // function because ipcMain.on(messages.NOTIFICATION_RESPONSE above - // calls into the callback without knowing an index. - const index = i - permissionCallbacks[message] = (buttonIndex, persist) => { - // hide the notification if this was triggered automatically - appActions.hideNotification(message) - const result = !!(buttonIndex) - response[index] = result - if (persist) { - // remember site setting for this host - appActions.changeSiteSetting(origin, permission + 'Permission', result, isPrivate) - } - if (response.length === permissionTypes.length) { - permissionCallbacks[message] = null - cb(response) + const responseAutoAdded = responseSizeThisIteration !== response.length + if (!responseAutoAdded) { + appActions.showNotification({ + buttons: [ + {text: locale.translation('deny')}, + {text: locale.translation('allow')} + ], + frameOrigin: getOrigin(mainFrameUrl), + options: { + persist: !!origin, + index: i + }, + message + }) + + // Use a closure here for the index instead of passing an index to the + // function because ipcMain.on(messages.NOTIFICATION_RESPONSE above + // calls into the callback without knowing an index. + const index = i + permissionCallbacks[message] = (buttonIndex, persist) => { + // hide the notification if this was triggered automatically + appActions.hideNotification(message) + const result = !!(buttonIndex) + response[index] = result + if (persist) { + // remember site setting for this host + appActions.changeSiteSetting(origin, permission + 'Permission', result, isPrivate) + } + if (response.length === permissionTypes.length) { + permissionCallbacks[message] = null + cb(response) + } } } } + if (response.length === permissionTypes.length) { + cb(response) + } }) }