[Merge] lp:~osomon/webbrowser-app/remove-formFactor into lp:webbrowser-app

Chris Coulson chris.coulson at canonical.com
Tue Feb 9 22:53:53 UTC 2016


I've added a comment inline to this

Diff comments:

> 
> === modified file 'src/app/webcontainer/WebViewImplOxide.qml'
> --- src/app/webcontainer/WebViewImplOxide.qml	2016-01-08 16:09:01 +0000
> +++ src/app/webcontainer/WebViewImplOxide.qml	2016-02-09 22:09:39 +0000
> @@ -328,13 +324,12 @@
>      }
>  
>      onGeolocationPermissionRequested: {
> -        if (formFactor == "desktop") {
> +        if (__runningConfined) {
> +            // When running confined, querying the location service will trigger
> +            // a system prompt (trust store), so no need for a custom one.
> +            request.accept()

I wish I'd noticed this before, but this isn't really safe. The trust store prompt is there to give permission to the application whereas this is for giving permission to specific domains. Whilst I appreciate we want to avoid 2 prompts, we shouldn't really give permission to all domains once an app is permitted to use the location service.

The problem with this approach is that any content loaded in a subframe by a webapp is silently permitted to use the location service, which I don't think is desired. It would be better to check the request origin matches that of the webapp before accepting without prompting.

> +        } else {
>              requestGeolocationPermission(request)
> -        } else {
> -            // On devices where webapps are confined, trying to access the
> -            // location service will trigger a system prompt from the trust
> -            // store, so we don’t need a custom prompt.
> -            request.accept()
>          }
>      }
>  


-- 
https://code.launchpad.net/~osomon/webbrowser-app/remove-formFactor/+merge/285539
Your team Ubuntu Phablet Team is requested to review the proposed merge of lp:~osomon/webbrowser-app/remove-formFactor into lp:webbrowser-app.



More information about the Ubuntu-reviews mailing list