1313242 - Prevent "Always allow" for insecure geolocation permission prompts

Status: RESOLVED INVALID

(Core :: DOM: Geolocation, defect, P3)

Description

[Matthew N. [:MattN]]

If we're not ready to implement bug 1072859 yet (which will break sites), we could at least remove the "Always allow" option when on HTTP. This could even be uplifted as it provides a potential annoyance instead of bustage.

The risk is that the lack of "Always allow" will lead to permission prompt fatigue.

1) The simplest incremental change would be to change GeolocationPermissionPrompt.promptActions to check for "https" instead of != "file" for the always case: https://dxr.mozilla.org/mozilla-central/rev/f9f3cc95d7282f1fd83f66dd74acbcdbfe821915/browser/modules/PermissionUI.jsm#459-462

The "never" case should probably use the existing logic.

If we want want to actually check for "secure contexts" instead of just "https" then we would probably need to extend nsIContentPermissionRequest to include whether the request was from a secure context. 

2) We would probably want a migration to drop exiting non-https permission manager records.

Comment 1

[Andrew Overholt [:overholt]]

jdm, what do you think? Do you have time to work on this?

Comment 2

[Josh Matthews [:jdm]]

Attachment: Do not allow storing permanent permissions for geolocation prompts for insecure origins

Comment 3

[Josh Matthews [:jdm]]

Yes.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 8809149 [details] [diff] [review]
Do not allow storing permanent permissions for geolocation prompts for insecure origins

Review of attachment 8809149 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/cookie/nsPermissionManager.cpp
@@ +1409,5 @@
> +        rv = stmtDeleteInsecure->BindInt32ByIndex(1, nsIPermissionManager::ALLOW_ACTION);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        bool hasResult;
> +        rv = stmtDeleteInsecure->ExecuteStep(&hasResult);

Do you care about hasResult? Some functions that call ExecuteStep() also MOZ_ASSERT(hasResult).

::: extensions/cookie/test/unit/test_permmanager_migrate_8-9.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Add "use strict"?

@@ +97,5 @@
> +  let expected = [
> +    ["https://foo.com", "geo", 1, 0, 0, 0],
> +    ["http://foo.com", "popup", 1, 0, 0, 0],
> +    ["http://bar.com", "geo", 1, 1, 0, 0],
> +    ["http://baz.com", "geo", 2, 0, 0, 0],

Is it worth adding test cases for subdomain geo permissions?

Comment 5

[Josh Matthews [:jdm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcc5defaa9ae

Comment 6

[Andrea Marchesini [:baku]]

Comment on attachment 8809149 [details] [diff] [review]
Do not allow storing permanent permissions for geolocation prompts for insecure origins

Review of attachment 8809149 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/cookie/nsPermissionManager.cpp
@@ +1412,5 @@
> +        bool hasResult;
> +        rv = stmtDeleteInsecure->ExecuteStep(&hasResult);
> +        NS_ENSURE_SUCCESS(rv, rv);
> +
> +        rv = mDBConn->SetSchemaVersion(9);

10

Comment 7

[Michelangelo De Simone [Offline] [:mds]]

As the patch for bug 1072859 has finally landed, I guess we don't need this anymore.