Closed Bug 1394134 Opened 2 years ago Closed 2 years ago

Several API permissions (including "proxy") are granted without prompting the user

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox55 wontfix, firefox56 verified, firefox57 verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox55 --- wontfix
firefox56 --- verified
firefox57 --- verified

People

(Reporter: Tomislav, Assigned: Tomislav)

References

Details

Attachments

(3 files)

The list in [1] needs to be reviewed and pruned.
Blocks: 1337938
Copying here to document what we need to do with each of these permissions in one place.  Here is my take:

>  activeTab 
>    -- (prompt not needed)
>  alarms 
>    --
>  browsingData 
>    -- (Chrome doesn't)
>  contextMenus 
>    --
>  contextualIdentities
>    ?
>  cookies 
>    -- (not needed since it also requires hosts permissions)
>  downloads.open 
>    probably needs a prompt, something like "Open files downloaded to your computer"
>  downloads.shelf 
>    -- we don't have a "downloads shelf", should probably remove this?
>  geckoProfiler 
>    -- (internal?)
>  identity 
>    unsupported but planned.  need to not forget to add it! ;)
>  idle 
>    --
>  menus 
>    --
>  proxy 
>    clearly needs prompting, something like "Control Firefox proxy settings"
>  storage 
>    --
>  theme 
>    --
>  webRequest 
>    -- (also requires hosts permissions, so similar to cookies?  also Chrome doesn't)
>  webRequestBlocking
>    -- (ditto)

Also, there doesn't seem to be any permission related to devtools, which doesn't seem right.

Here what Chrome prompts for:  https://developer.chrome.com/extensions/permission_warnings

Andrew and Kris, can you please go over the list and confirm/correct where needed?

(Btw, I'm inclined to ask for uplift here)
Flags: needinfo?(aswan)
Flags: needinfo?(kmaglione+bmo)
contextualIdentities already has bug 1333253
I agree that proxy (!!) and downloads.open should have strings
Also browsingData and theme seem like good candidates to have strings, I don't think any others should be prompted, for the reasons you laid out.
Flags: needinfo?(aswan)
Just a quick ni over to security, :cr your team did a review of the APIs and permission models I think? Anything to add on this one?
Flags: needinfo?(cr)
Blocks: 1394553
Theme seems like the least dangerous, and most obvious to users of all the permissions.


Scott, can you please provide final wording for these permission prompts:

proxy:  
  "Control Firefox proxy settings"
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/proxy

downloads.open:
  "Open files downloaded to your computer"
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/downloads/open

browsingData:
  "Clear recent history, cookies and related browsing data"
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/browsingData

manifest:devtools_page
  "Extend developer tools, to inspect and affect loaded pages"
  https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Extending_the_developer_tools
Assignee: nobody → tomica
Status: NEW → ASSIGNED
Flags: needinfo?(sdevaney)
proxy: 
"Control browser proxy settings"

downloads.open: 
"Open files downloaded to your computer"

browsingData: 
"Clear recent browsing history, cookies, and related data"

manifest:devtools_page (the example copy had 'inspect and affect' loaded pages but itsn't this effective the same as "read and write"? If so we should use that language convention to remain consistent; but if I'm wrong here please advise): 
"Extend developer tools to read and write loaded pages"
Flags: needinfo?(sdevaney)
(In reply to sdevaney from comment #6)
> "Extend developer tools to read and write loaded pages"

This sounds weird to me without any prepositions.  

I just noticed the wording used for the Find api is "open tabs", which sounds better to me than my original "loaded pages".

Also, since this is semantically similar to the host permission, I propose using similar wording:

"Extend developer tools to access your data in open tabs"?
Flags: needinfo?(sdevaney)
> "Extend developer tools to access your data in open tabs"?

Love it. Let's go with that. Thanks.
Flags: needinfo?(sdevaney)
Attachment #8902052 - Flags: review?(aswan)
Thanks Scott.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8902052 [details]
Bug 1394134 - Add permission strings for proxy, browsingData and downloads.open

Approval Request Comment
>[Feature/Bug causing the regression]:
Bug 1295807 - Support proxy configuration from WebExtensions

>[User impact if declined]:
Extensions are silently given permission to control proxy settings without prompting the user

>[Is this code covered by automated tests?]:
This isn't changing any code, just adding permission prompt strings.

>[Has the fix been verified in Nightly?]:
No

>[Needs manual test from QE? If yes, steps to reproduce]: 
Yes.  STR: Install an extension that uses the Proxy API from AMO, verify that you are prompted.

>[List of other uplifts needed for the feature/fix]:
None

>[Is the change risky?]:
Not risky

>[Why is the change risky/not risky?]:
This doesn't change any code.

>[String changes made/needed]:
This patch is all about adding 3 new prompt strings.  If other locales are not updated to include them, the fallback is to just not prompt for permissions, which is exactly the current behavior.
Attachment #8902052 - Flags: approval-mozilla-beta?
Comment on attachment 8902052 [details]
Bug 1394134 - Add permission strings for proxy, browsingData and downloads.open

https://reviewboard.mozilla.org/r/173464/#review178888

Looks okay to me but you should have an l10n expert (eg :flod) take a look.
Attachment #8902052 - Flags: review?(aswan) → review+
Attachment #8902052 - Flags: review?(francesco.lodolo)
Comment on attachment 8902052 [details]
Bug 1394134 - Add permission strings for proxy, browsingData and downloads.open

https://reviewboard.mozilla.org/r/173464/#review179034

The strings look good, it's an r+ for that. 

Landing string changes in Beta so far ahead in the cycle is a whole different topic.

This bug only adds strings. What's being done to avoid similar cases in the future? 

Is it possible to hard-code these strings for Beta without touching browser.properties? Note that this also relates to https://bugzilla.mozilla.org/show_bug.cgi?id=1332144#c184

::: browser/locales/en-US/chrome/browser/browser.properties:100
(Diff revision 1)
>  webextPerms.optionalPermsDeny.label=Deny
>  webextPerms.optionalPermsDeny.accessKey=D
>  
>  webextPerms.description.bookmarks=Read and modify bookmarks
>  webextPerms.description.browserSettings=Read and modify browser settings
> +webextPerms.description.browsingData=Clear recent browsing history, cookies, and related data

Why is the pref called browsingData if the description is about deleting data?
Attachment #8902052 - Flags: review?(francesco.lodolo) → review+
(In reply to Francesco Lodolo [:flod] from comment #13)
> Landing string changes in Beta so far ahead in the cycle is a whole
> different topic.
> 
> This bug only adds strings. What's being done to avoid similar cases in the
> future? 

A brief history (from the last 10 days :)

 - A new Find api got backed out when we noticed it landed without a permission string (bug 1332144 comment #139).
 - To avoid that happening again, I added a new test to verify all new permissions have strings (bug 1337938).
 - While working on that, I noticed a few other were missing strings, most notably the "proxy" permission (this bug).

All this is rather unfortunate, and we wouldn't normally ask for uplifting localization strings to beta, as I understand that is very disruptive to the localization process.  But in this case, I would argue this is equivalent to a security issue (extensions being granted permissions silently), so I'm hoping you would make an exception.


> Is it possible to hard-code these strings for Beta without touching
> browser.properties? Note that this also relates to

Either I don't understand the question.  Is your suggestion to hardcode to english permission strings since the localization will be missing (or as a fallback)?


> Why is the pref called browsingData if the description is about deleting
> data?

(I'm guessing) for sanity reasons, all the prefs are named the same as the corresponding permissions.  And the api/permission is named like that for Chrome compatibility reasons (https://developers.chrome.com/extensions/browsingData).
Ni for Andrew to make sure I'm not talking out of turn.
Flags: needinfo?(aswan)
(In reply to Tomislav Jovanovic :zombie from comment #14)
> All this is rather unfortunate, and we wouldn't normally ask for uplifting
> localization strings to beta, as I understand that is very disruptive to the
> localization process.  But in this case, I would argue this is equivalent to
> a security issue (extensions being granted permissions silently), so I'm
> hoping you would make an exception.

It is indeed. Good to know that there are tests to cover it now. 

Did we ship those permissions in 55 or they landed only in 56?

> Either I don't understand the question.  Is your suggestion to hardcode to
> english permission strings since the localization will be missing (or as a
> fallback)?

When you land strings in Beta, you "generate" activity on the Beta branch for all locales, forcing l10n-drivers to manually review all the sign-offs (~90) to make sure that real changes, e.g. translations for those 3 strings, are not missed. It's not a fast process, imagine if it happens multiple times a cycle. This is the 2nd string freeze break in a week, the other one was for a security bug in Fennec.

Additionally, landing strings in Beta is breaking our "pact" with localizers, piling up frustration: Beta is supposed to be a stable string froze environment, once they're done with translation and testing they shouldn't need to touch Beta anymore.

All this to explain that yes, we can land strings in Beta, but it's not something to be done lightly, especially 2 weeks from end of the cycle. 

Also note that the final decision is not l10n's, it's release drivers (and release owners for 56) who will examine the beta-approval flag.
(In reply to Tomislav Jovanovic :zombie from comment #14)
> > Why is the pref called browsingData if the description is about deleting
> > data?
> 
> (I'm guessing) for sanity reasons, all the prefs are named the same as the
> corresponding permissions.  And the api/permission is named like that for
> Chrome compatibility reasons
> (https://developers.chrome.com/extensions/browsingData).

Its not just for sanity, we generate the l10n ids from the named permissions.  I know that's generally frowned upon but I think the alternative is some big ugly switch like:
```
if (permission == "history") {
  id = "permissions.history";
} else if (permission == "downloads" {
  id = "permissions.downloads";
} else if (permission == "browsingData") {
  id = "permissions.broswingData";
} else (...15 or so more clauses...)
```

If the question is why is the permission itself named browsingData, that's a fine question, we can add it to the long list of questionable decisions the designers of the Chrome extensions system made.
Flags: needinfo?(aswan)
(In reply to Andrew Swan [:aswan] from comment #17)
> Its not just for sanity, we generate the l10n ids from the named
> permissions.  I know that's generally frowned upon but I think the
> alternative is some big ugly switch like:

We need to come up with a system that allows overrides, the sooner the better. Bug 1332144 and bug 1394740 show that we need that already.
Google's list reflects what we also found in terms of API risks afaics, so if you're using that as a guideline, we should be fine under the condition that users are clearly informed about the implications of those API permission + host permission combos.
Flags: needinfo?(cr)
(In reply to Francesco Lodolo [:flod] from comment #16)
> Did we ship those permissions in 55 or they landed only in 56?

The "proxy" api landed in 55 unfortunately.
 

> All this to explain that yes, we can land strings in Beta, but it's not
> something to be done lightly, especially 2 weeks from end of the cycle. 
> 
> Also note that the final decision is not l10n's, it's release drivers (and
> release owners for 56) who will examine the beta-approval flag.

In that case, I'm ok with providing a patch for beta with hardcoded strings if the release drivers decide that's the only way they will approve this.


(In reply to Francesco Lodolo [:flod] from comment #18)
> We need to come up with a system that allows overrides, the sooner the
> better. Bug 1332144 and bug 1394740 show that we need that already.

Filed bug 1394936, though I'm not sure where it stacks up in priority queue.
In the mean time, I'm landing this as is to Nightly.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f9e8818dc535 -d 341d93b956be: rebasing 416683:f9e8818dc535 "Bug 1394134 - Add permission strings for proxy, browsingData and downloads.open r=aswan,flod" (tip)
merging browser/locales/en-US/chrome/browser/browser.properties
warning: conflicts while merging browser/locales/en-US/chrome/browser/browser.properties! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by tomica@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6ae174ca6fe1
Add permission strings for proxy, browsingData and downloads.open r=aswan,flod
https://hg.mozilla.org/mozilla-central/rev/6ae174ca6fe1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
We could uplift this if it is en-US only with hard coded strings. I don't want to put any more strain on the l10n team at this stage of beta.
Attachment #8902899 - Flags: review?(kmaglione+bmo)
Attachment #8902899 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8902899 [details] [diff] [review]
bug_1394134_beta_fix.diff

Let's uplift this for beta 8. String hard coded for en-US only.
Attachment #8902899 - Flags: approval-mozilla-beta+
Comment on attachment 8902052 [details]
Bug 1394134 - Add permission strings for proxy, browsingData and downloads.open

superceded by the patch in https://bugzilla.mozilla.org/attachment.cgi?id=8902899&action=edit.
Attachment #8902052 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
After checking the bug on Firefox Nightly 57.0a1(20170905220108 / 20170906100107) and Beta 56.0b9 (20170903140023) under Wind 7 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.12.3 we noticed the following:

- Nightly: all the permissions are displayed excepting while installing from an external server on Ubuntu where none of these 3 new implemented permissions are listed in the pop-up - see line 54
- Beta: only proxy permissions is displayed across all platforms

All the testing results are tracked in https://public.etherpad-mozilla.org/p/1394134

The permissions prompted for an extension update case could not be check, because the Beta Unbranded build is not up to date - it is 56.0b5 (https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win64-add-on-devel/1503310677/). Could someone upgrade the unbranded build in order to complete the testing also for update case?
Flags: needinfo?(tomica)
> - Nightly: all the permissions are displayed excepting while installing from
> an external server on Ubuntu where none of these 3 new implemented
> permissions are listed in the pop-up - see line 54
This is very unexpected, there is no platform-specific code anywhere near permissions.

Can you please double check this test?  And post Nightly version and the content of browser.properties [1] from your browser?

1) chrome://browser/locale/browser.properties 


> - Beta: only proxy permissions is displayed across all platforms
This is expected, the patch that landed on beta (comment 27) only hardcoded the proxy permission.


> Could someone upgrade the unbranded build in
> order to complete the testing also for update case?
I don't know who can do that.
Flags: needinfo?(tomica) → needinfo?(cosmin.badescu)
Attached file browser.properties.txt
Sure, the Firefox 57.0a1 (20170905220108) is the one used for testing on Ubuntu.

The result is the same after the double check.

Please let me know if you need other info from me.
Flags: needinfo?(cosmin.badescu)
So this only makes sense if that is a build with a different locale (that's still English?) that didn't get the new strings yet?  Can you please post the link you used to download the the Nightly build (and try the EN-US one if it wasn't already)?


> https://addons-dev.allizom.org/en-US/firefox/addon/permissions-test3/
> PASS - permissions prompted from AMO 
> PASS - permissions prompted via sideloading method
> PASS - permissions prompted from local install from file or drag & drop
> FAIL - permissions prompted from an external server/thirdparty - 
>   https://drive.google.com/file/d/0B3Rw2hpbuNiLck84VzlkOUk2Ymc/view?usp=sharing - 
>   browsingData, downloads.open and proxy are not displayed 
>   https://www.dropbox.com/s/r37r8hzoepjvos0/Screenshot%20from%202017-09-06%2017-02-16.png?dl=0
> PASS - permissions prompted extension updates https://addons-dev.allizom.org/en-US/firefox/addon/permissions-testupdates/

This makes no sense.  Either the strings are present in the build, and you get prompted for them regardless of the method used, or they are not present (as in the attachment), and you shouldn't see them in any of the tests with the same build of Nightly.  

Can you please triple-confirm all those tests again are done with the same build of Nightly?
Flags: needinfo?(cosmin.badescu)
This issue is verified as fixed on Firefox Nightly 57.0a1(20170905220108 / 20170906220306 / 20170906100107) and Beta 56.0b9 (20170903140023) under Wind 7 64-bit, Ubuntu 12.04 64-bit and Mac OS X 10.12.3

After the third check on Ubuntu 12.04 64-bit, Nightly displayed all the permissions and Beta displayed the proxy permission.

The issue with the Ubuntu was that the option “Choose Helper Applications” did not open the Nightly from the right path.

Thanks for the help!

Please see the videos for Ubuntu.
Beta https://drive.google.com/file/d/0B3Rw2hpbuNiLYkV6bnJLcGplVWc/view?usp=sharing 
Nightly https://drive.google.com/file/d/0B3Rw2hpbuNiLNTdJRUtrekpMaFU/view?usp=sharing
Status: RESOLVED → VERIFIED
Flags: needinfo?(cosmin.badescu)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.