1331769 - Consider more clever checking for host permission changes

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P2)

Description

[Andrew Swan [:aswan]]

In the proposed patch for 1317470 we simply compute differences in the permissions a webextension declares in its manifest.  But this means that changing from a permission for "<all_urls>" to "https://*.somedomain.com" or from "https://*.somedomain.com" to "https://host.somedomain.com" will trigger prompts for new permissions when in fact those are examples of actually reducing the permissions of the extension.
If this is a big deal we should address it but I'm reluctant to write a bunch of extra code for this until we have reason to think its worth doing.

Comment 2

[Ian Moody [:Kwan] (UTC+1)]

Carrying over P2 from bug 1373434

Comment 3

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1331769 - Properly check whether new origin permissions are a subset of old origin permissions.

This prevents prompts occuring in cases where the new permissions are more
specific but still covered by the old permissions.
e.g.:
["<all_urls>"] -> ["<all_urls>", "*://*.example.com"]
["*://*.example.com"] -> ["http://subdomain.example.com"]

Review commit: https://reviewboard.mozilla.org/r/182404/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/182404/

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8910944 [details]
Bug 1331769 - Properly check whether new origin permissions are a subset of old origin permissions.

https://reviewboard.mozilla.org/r/182404/#review188098

Looks good (and thanks again!) but this also should have tests.

Comment 5

[Ian Moody [:Kwan] (UTC+1)]

Attachment: Bug 1331769 - Test that there is no prompt for origin permission narrowing.

Review commit: https://reviewboard.mozilla.org/r/183018/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183018/

Comment 6

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8910944 [details]
Bug 1331769 - Properly check whether new origin permissions are a subset of old origin permissions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/182404/diff/1-2/

Comment 7

[Ian Moody [:Kwan] (UTC+1)]

Some questions about testing:
at the moment it only tests ["<all_urls>"] -> ["*://*.example.com/*"] narrowing.  Would it be worth adding another extension to test other examples, e.g.:
["*://*.example.com/*", "http://*.example2.com/*", "http://www.example3.com/*", "https://*/*"] ->
["http://*.example.com/*", "http://subdomain.example2.com/*", "http://www.example3.com/path/*", "https://*.example4.com/*"]

Do the extensions need to be signed?  The tests seem to work fine without them being, but it appears the other extensions in the directory are.
If so, is signing them locally with web-ext sign <keys> fine, or do they need special signing? (especially as they mention the mozilla domain in their IDs)

Comment 8

[Andrew Swan [:aswan]]

(In reply to Ian Moody [:Kwan] from comment #7)
> Some questions about testing:
> at the moment it only tests ["<all_urls>"] -> ["*://*.example.com/*"]
> narrowing.  Would it be worth adding another extension to test other
> examples, e.g.:
> ["*://*.example.com/*", "http://*.example2.com/*",
> "http://www.example3.com/*", "https://*/*"] ->
> ["http://*.example.com/*", "http://subdomain.example2.com/*",
> "http://www.example3.com/path/*", "https://*.example4.com/*"]

I do think some additional tests would be good here, though this is really about testing MatchPatternSet.subsumes().  I think ideally we would put those tests in:
http://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_MatchPattern.js
It would be good to have systematic tests for specific cases where subsumes() should return true but also cases where it should return false (e.g., "domain.com" -> "*.domain.com").
Anyhow, that can happen in a follow-up bug (if its something you're interested in working on, that would be great!)

> Do the extensions need to be signed?  The tests seem to work fine without
> them being, but it appears the other extensions in the directory are.
> If so, is signing them locally with web-ext sign <keys> fine, or do they
> need special signing? (especially as they mention the mozilla domain in
> their IDs)

Since the tests in this directory were first created we landed changes that let us disable signing in the test environment, which means that they don't need to be signed, but the xpinstall.signatures.required preference needs to be set to false or the tests will fail when this code gets to beta.
And since the extensions don't need to be signed, that means we can skip checking in xpis altogether and create extensions on the fly with `AddonTestUtils.createTempWebExtensionFile()`.  That should make the tests much more readable since they don't rely on the contents of zip files.  I'd like to get all the tests in this directory switched over to that method but switching them all is beyond the scope of this bug.  I'll leave it up to you if you prefer to make that change now or just land the xpis and let them get converted when the rest of the directory gets converted.  Of course, if you want to work on the follow-up to convert the existing tests, that would be great!

Comment 9

[Ian Moody [:Kwan] (UTC+1)]

(In reply to Andrew Swan [:aswan] from comment #8)
> (In reply to Ian Moody [:Kwan] from comment #7)
> > Some questions about testing:
> > at the moment it only tests ["<all_urls>"] -> ["*://*.example.com/*"]
> > narrowing.  Would it be worth adding another extension to test other
> > examples, e.g.:
> > ["*://*.example.com/*", "http://*.example2.com/*",
> > "http://www.example3.com/*", "https://*/*"] ->
> > ["http://*.example.com/*", "http://subdomain.example2.com/*",
> > "http://www.example3.com/path/*", "https://*.example4.com/*"]
> 
> I do think some additional tests would be good here, though this is really
> about testing MatchPatternSet.subsumes().  I think ideally we would put
> those tests in:
> http://searchfox.org/mozilla-central/source/toolkit/components/extensions/
> test/xpcshell/test_MatchPattern.js
> It would be good to have systematic tests for specific cases where
> subsumes() should return true but also cases where it should return false
> (e.g., "domain.com" -> "*.domain.com").
> Anyhow, that can happen in a follow-up bug (if its something you're
> interested in working on, that would be great!)
Yep, I've filed bug 1403224 and I'll submit a patch.

> > Do the extensions need to be signed?  The tests seem to work fine without
> > them being, but it appears the other extensions in the directory are.
> > If so, is signing them locally with web-ext sign <keys> fine, or do they
> > need special signing? (especially as they mention the mozilla domain in
> > their IDs)
> 
> Since the tests in this directory were first created we landed changes that
> let us disable signing in the test environment, which means that they don't
> need to be signed, but the xpinstall.signatures.required preference needs to
> be set to false or the tests will fail when this code gets to beta.
> And since the extensions don't need to be signed, that means we can skip
> checking in xpis altogether and create extensions on the fly with
> `AddonTestUtils.createTempWebExtensionFile()`.  That should make the tests
> much more readable since they don't rely on the contents of zip files.  I'd
> like to get all the tests in this directory switched over to that method but
> switching them all is beyond the scope of this bug.  I'll leave it up to you
> if you prefer to make that change now or just land the xpis and let them get
> converted when the rest of the directory gets converted.  Of course, if you
> want to work on the follow-up to convert the existing tests, that would be
> great!
Okay I've added setting the signatures.required pref to false to the patch.
While not having to check in unreadable files is a definite plus, I'll just leave them as XPIs for now since changing the tests enough to create these ones on the fly seems as much work as changing the test to create all three (legacy and update_perms).  Additionally I'm not sure how to serve generated files in a mochitest to the updater, is there some equivalent to xpcshell's createHttpServer?

Comment 10

[Ian Moody [:Kwan] (UTC+1)]

Comment on attachment 8911595 [details]
Bug 1331769 - Test that there is no prompt for origin permission narrowing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183018/diff/1-2/

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8910944 [details]
Bug 1331769 - Properly check whether new origin permissions are a subset of old origin permissions.

https://reviewboard.mozilla.org/r/182404/#review188864

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8910944 [details]
Bug 1331769 - Properly check whether new origin permissions are a subset of old origin permissions.

https://reviewboard.mozilla.org/r/182404/#review188866

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8911595 [details]
Bug 1331769 - Test that there is no prompt for origin permission narrowing.

https://reviewboard.mozilla.org/r/183018/#review188868

Comment 14

[Ian Moody [:Kwan] (UTC+1)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b732f1db720
Properly check whether new origin permissions are a subset of old origin permissions. r=aswan
https://hg.mozilla.org/integration/autoland/rev/e48250ab7382
Test that there is no prompt for origin permission narrowing. r=aswan

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1b732f1db720
https://hg.mozilla.org/mozilla-central/rev/e48250ab7382

Comment 17

[CosminB]

Attachment: hostPrompted.gif

I observed for the extension hostpermissionstest0 if you upgrade from v1.1 to v1.2 will display the permission string for <all_urls>, is this expected?

Checked on Firefox 58.0a1 (20171002220204) under Wind 7 64-bit and Ubuntu 16.04 32-bit.

Extension used:
https://addons-dev.allizom.org/en-US/firefox/addon/hostpermissionstest0/versions/
v1.0 “<all_urls>” -> v1.1 “*://*.example.com/*” -> v1.2 "http://*.example2.com/*", "http://www.example3.com/*", "https://*/*"

Comment 18

[Ian Moody [:Kwan] (UTC+1)]

(In reply to CosminB from comment #17)
> Created attachment 8914672 [details]
> hostPrompted.gif
> 
> I observed for the extension hostpermissionstest0 if you upgrade from v1.1
> to v1.2 will display the permission string for <all_urls>, is this expected?
Isn't that because "https://*/*" is also presented as all websites?  I don't think the prompt makes protocol distinctions to avoid confusing the average user, it only mentions domains.  In fact that result should have been true before this patch, as far as I can deduce.

Comment 19

[CosminB]

Attachment: hostNotPrompt.gif

Yes, you are right Ian Moody.
Thanks for the help!

This issue is verified as fixed on Firefox 58.0a1 (20171002220204) under Wind 7 64-bit and Ubuntu 16.04 32-bit.

Upgrading to a new version, the host permissions will not trigger a new prompt. 

Extension used:
https://addons-dev.allizom.org/en-US/firefox/addon/hostpermissionstest0/versions/
v1.0 “<all_urls>” -> 
v1.1 “*://*.example.com/*” -> 
v1.2 "http://*.example2.com/*", "http://www.example3.com/*", "https://*/*"

https://addons-dev.allizom.org/en-US/firefox/addon/hostpermissiont/versions/
v1.0 "*://*.example.com/*", "http://*.example2.com/*", "http://www.example3.com/*", "https://*/*" -> 
v1.1 "http://*.example.com/*", "http://subdomain.example2.com/*", "http://www.example3.com/path/*", "https://*.example4.com/*" 

Please see the attached video.