From 771e34a53a03bfaa8299a0eb28589baeae756a7b Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 7 Dec 2020 15:44:56 -0800 Subject: [PATCH] feat: route frame based permission checks through our permission check handler (#19903) * feat: route frame based permission checks through our permission check handler * docs: add change to setPermissionCheckHandler to breaking changes doc --- docs/api/session.md | 27 ++++++++------ docs/breaking-changes.md | 22 ++++++++++++ shell/browser/electron_permission_manager.cc | 38 ++++++++++++++++---- 3 files changed, 70 insertions(+), 17 deletions(-) diff --git a/docs/api/session.md b/docs/api/session.md index fceafaee1a..035dccaee6 100644 --- a/docs/api/session.md +++ b/docs/api/session.md @@ -500,6 +500,7 @@ win.webContents.session.setCertificateVerifyProc((request, callback) => { * `pointerLock` - Request to directly interpret mouse movements as an input method. Click [here](https://developer.mozilla.org/en-US/docs/Web/API/Pointer_Lock_API) to know more. * `fullscreen` - Request for the app to enter fullscreen mode. * `openExternal` - Request to open links in external applications. + * `unknown` - An unrecognized permission request * `callback` Function * `permissionGranted` Boolean - Allow or deny the permission. * `details` Object - Some properties are only available on certain permission types. @@ -511,7 +512,9 @@ win.webContents.session.setCertificateVerifyProc((request, callback) => { Sets the handler which can be used to respond to permission requests for the `session`. Calling `callback(true)` will allow the permission and `callback(false)` will reject it. -To clear the handler, call `setPermissionRequestHandler(null)`. +To clear the handler, call `setPermissionRequestHandler(null)`. Please note that +you must also implement `setPermissionCheckHandler` to get complete permission handling. +Most web APIs do a permission check and then make a permission request if the check is denied. ```javascript const { session } = require('electron') @@ -527,28 +530,32 @@ session.fromPartition('some-partition').setPermissionRequestHandler((webContents #### `ses.setPermissionCheckHandler(handler)` * `handler` Function\ | null - * `webContents` [WebContents](web-contents.md) - WebContents checking the permission. Please note that if the request comes from a subframe you should use `requestingUrl` to check the request origin. + * `webContents` ([WebContents](web-contents.md) | null) - WebContents checking the permission. Please note that if the request comes from a subframe you should use `requestingUrl` to check the request origin. Cross origin sub frames making permission checks will pass a `null` webContents to this handler. You should use `embeddingOrigin` and `requestingOrigin` to determine what origin the owning frame and the requesting frame are on respectively. * `permission` String - Type of permission check. Valid values are `midiSysex`, `notifications`, `geolocation`, `media`,`mediaKeySystem`,`midi`, `pointerLock`, `fullscreen`, `openExternal`, or `serial`. * `requestingOrigin` String - The origin URL of the permission check * `details` Object - Some properties are only available on certain permission types. - * `securityOrigin` String - The security origin of the `media` check. - * `mediaType` String - The type of media access being requested, can be `video`, + * `embeddingOrigin` String (optional) - The origin of the frame embedding the frame that made the permission check. Only set for cross-origin sub frames making permission checks. + * `securityOrigin` String (optional) - The security origin of the `media` check. + * `mediaType` String (optional) - The type of media access being requested, can be `video`, `audio` or `unknown` - * `requestingUrl` String - The last URL the requesting frame loaded + * `requestingUrl` String (optional) - The last URL the requesting frame loaded. This is not provided for cross-origin sub frames making permission checks. * `isMainFrame` Boolean - Whether the frame making the request is the main frame Sets the handler which can be used to respond to permission checks for the `session`. -Returning `true` will allow the permission and `false` will reject it. +Returning `true` will allow the permission and `false` will reject it. Please note that +you must also implement `setPermissionRequestHandler` to get complete permission handling. +Most web APIs do a permission check and then make a permission request if the check is denied. To clear the handler, call `setPermissionCheckHandler(null)`. ```javascript const { session } = require('electron') -session.fromPartition('some-partition').setPermissionCheckHandler((webContents, permission) => { - if (webContents.getURL() === 'some-host' && permission === 'notifications') { - return false // denied +const url = require('url') +session.fromPartition('some-partition').setPermissionCheckHandler((webContents, permission, requestingOrigin) => { + if (new URL(requestingOrigin).hostname === 'some-host' && permission === 'notifications') { + return true // granted } - return true + return false // denied }) ``` diff --git a/docs/breaking-changes.md b/docs/breaking-changes.md index fd2d63aacd..9db323fe5d 100644 --- a/docs/breaking-changes.md +++ b/docs/breaking-changes.md @@ -14,6 +14,28 @@ This document uses the following convention to categorize breaking changes: ## Planned Breaking API Changes (13.0) +### API Changed: `session.setPermissionCheckHandler(handler)` + +The `handler` methods first parameter was previously always a `webContents`, it can now sometimes be `null`. You should use the `requestingOrigin`, `embeddingOrigin` and `securityOrigin` properties to respond to the permission check correctly. As the `webContents` can be `null` it can no longer be relied on. + +```js +// Old code +session.setPermissionCheckHandler((webContents, permission) => { + if (webContents.getURL().startsWith('https://google.com/') && permission === 'notification') { + return true + } + return false +}) + +// Replace with +session.setPermissionCheckHandler((webContents, permission, requestingOrigin) => { + if (new URL(requestingOrigin).hostname === 'google.com' && permission === 'notification') { + return true + } + return false +}) +``` + ### Removed: `shell.moveItemToTrash()` The deprecated synchronous `shell.moveItemToTrash()` API has been removed. Use diff --git a/shell/browser/electron_permission_manager.cc b/shell/browser/electron_permission_manager.cc index c06828be6c..324b8f3da7 100644 --- a/shell/browser/electron_permission_manager.cc +++ b/shell/browser/electron_permission_manager.cc @@ -224,7 +224,12 @@ blink::mojom::PermissionStatus ElectronPermissionManager::GetPermissionStatus( content::PermissionType permission, const GURL& requesting_origin, const GURL& embedding_origin) { - return blink::mojom::PermissionStatus::GRANTED; + base::DictionaryValue details; + details.SetString("embeddingOrigin", embedding_origin.spec()); + bool granted = CheckPermissionWithDetails(permission, nullptr, + requesting_origin, &details); + return granted ? blink::mojom::PermissionStatus::GRANTED + : blink::mojom::PermissionStatus::DENIED; } int ElectronPermissionManager::SubscribePermissionStatusChange( @@ -247,13 +252,28 @@ bool ElectronPermissionManager::CheckPermissionWithDetails( return true; } auto* web_contents = - content::WebContents::FromRenderFrameHost(render_frame_host); + render_frame_host + ? content::WebContents::FromRenderFrameHost(render_frame_host) + : nullptr; auto mutable_details = details == nullptr ? base::DictionaryValue() : details->Clone(); - mutable_details.SetStringKey("requestingUrl", - render_frame_host->GetLastCommittedURL().spec()); - mutable_details.SetBoolKey("isMainFrame", - render_frame_host->GetParent() == nullptr); + if (render_frame_host) { + mutable_details.SetStringKey( + "requestingUrl", render_frame_host->GetLastCommittedURL().spec()); + } + mutable_details.SetBoolKey( + "isMainFrame", + render_frame_host && render_frame_host->GetParent() == nullptr); + switch (permission) { + case content::PermissionType::AUDIO_CAPTURE: + mutable_details.SetStringKey("mediaType", "audio"); + break; + case content::PermissionType::VIDEO_CAPTURE: + mutable_details.SetStringKey("mediaType", "video"); + break; + default: + break; + } return check_handler_.Run(web_contents, permission, requesting_origin, mutable_details); } @@ -263,7 +283,11 @@ ElectronPermissionManager::GetPermissionStatusForFrame( content::PermissionType permission, content::RenderFrameHost* render_frame_host, const GURL& requesting_origin) { - return blink::mojom::PermissionStatus::GRANTED; + base::DictionaryValue details; + bool granted = CheckPermissionWithDetails(permission, render_frame_host, + requesting_origin, &details); + return granted ? blink::mojom::PermissionStatus::GRANTED + : blink::mojom::PermissionStatus::DENIED; } } // namespace electron