From c80b19015f66e9e40afc20d11892c66dcc0ffa82 Mon Sep 17 00:00:00 2001 From: bridiver Date: Mon, 11 Sep 2017 10:12:56 -0700 Subject: [PATCH 1/3] handle an array of permission types fix #10864 requires muon 4.3.19+ --- app/filtering.js | 140 +++++++++--------- .../components/main/notificationBar.js | 20 ++- 2 files changed, 87 insertions(+), 73 deletions(-) diff --git a/app/filtering.js b/app/filtering.js index cc83ee86492..89a2a0930df 100644 --- a/app/filtering.js +++ b/app/filtering.js @@ -365,7 +365,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: { @@ -395,34 +395,7 @@ function registerPermissionHandler (session, partition) { } } - if (!permissions[permission]) { - console.log('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 @@ -450,48 +423,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) + } - const message = locale.translation('permissionMessage').replace(/{{\s*host\s*}}/, origin).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: true - }, - 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/notificationBar.js b/app/renderer/components/main/notificationBar.js index 979de732847..b41f1942677 100644 --- a/app/renderer/components/main/notificationBar.js +++ b/app/renderer/components/main/notificationBar.js @@ -26,14 +26,22 @@ const globalStyles = require('../styles/global') class NotificationItem extends ImmutableComponent { clickHandler (buttonIndex, e) { - const nonce = this.props.detail.get('options').get('nonce') + const nonce = this.props.detail.getIn(['options', 'nonce']) + const index = this.props.detail.getIn(['options', 'index']) if (nonce) { - ipc.emit(messages.NOTIFICATION_RESPONSE + nonce, {}, - this.props.detail.get('message'), - buttonIndex, this.checkbox ? this.checkbox.checked : false) + ipc.send( + messages.NOTIFICATION_RESPONSE + nonce, {}, + this.props.detail.get('message'), + buttonIndex, this.checkbox ? this.checkbox.checked : false, + index + ) } else { - ipc.send(messages.NOTIFICATION_RESPONSE, this.props.detail.get('message'), - buttonIndex, this.checkbox ? this.checkbox.checked : false) + ipc.send( + messages.NOTIFICATION_RESPONSE, + this.props.detail.get('message'), + buttonIndex, this.checkbox ? this.checkbox.checked : false, + index + ) } } From bd8f74569516de176571a56ef076e01d97d8c868 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Wed, 13 Sep 2017 09:39:04 -0400 Subject: [PATCH 2/3] 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 89a2a0930df..6e2c43fd3ae 100644 --- a/app/filtering.js +++ b/app/filtering.js @@ -426,41 +426,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 29010ccaaec904d63cea29d9c6ff7bdec63fee08 Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Wed, 13 Sep 2017 11:23:09 -0400 Subject: [PATCH 3/3] 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 6e2c43fd3ae..799e900487b 100644 --- a/app/filtering.js +++ b/app/filtering.js @@ -463,7 +463,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({ @@ -479,7 +479,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)